binary attributes

630 views
Skip to first unread message

jon.schell

unread,
Feb 9, 2011, 8:26:48 PM2/9/11
to Android Linux Kernel Development
I'm creating a binary attribute (struct bin_attribute) for a sys/class/
led driver, and it seems to be correct, but when I echo data to the
write function, it seems to get called 11 times. Here's what I have
and did:

static ssize_t my_write(struct kobject *kobj, struct bin_attribute
*attr, char *buf, loff_t pos, size_t size)
{
print_string("called my_write function correctly.");
return 0;
}

static struct bin_attribute test_attr = {
.attr = {
.name = "test_data",
.mode = S_IWUSR | S_IWGRP | S_IWOTH,
.owner = THIS_MODULE,
},
.size = 100,
.write = my_write,
.read = NULL,
};

static int __init led_module_init(void)
{
error = sysfs_create_bin_file(&(test_led.dev->kobj), &test_attr);
if (error)
sysfs_remove_bin_file(&(test_led.dev->kobj), &test_attr);
}

And then from adb shell, I just did "echo 1 > sys/class/leds/test_led/
test_data" and it printed the test message 11 times. When I put a
test message in the brightness_set function, it only printed that
once, as expected. Is there something funny about bin_attribute that
would be doing this?

Greg KH

unread,
Feb 9, 2011, 10:02:24 PM2/9/11
to android...@googlegroups.com
On Wed, Feb 9, 2011 at 5:26 PM, jon.schell <jon.s...@kyocera.com> wrote:
> I'm creating a binary attribute (struct bin_attribute) for a sys/class/
> led driver,

Wait, stop right there. Binary sysfs attributes are for sending/receiving
binary data that is not intrepreted/manipulated by the kernel at all.

And LED devices do not seem to fall into the category that needs this
type of sysfs file, so please do not create it.

> and it seems to be correct, but when I echo data to the
> write function, it seems to get called 11 times.  Here's what I have
> and did:
>
> static ssize_t my_write(struct kobject *kobj, struct bin_attribute
> *attr, char *buf, loff_t pos, size_t size)
> {
>    print_string("called my_write function correctly.");
>    return 0;
> }

Did you read the documentation on what your write function should return?
(hint, 0 is not what you think it means here).

Fix this and your problem will be solved.

But again, don't create a binary sysfs attribute, that is almost
never what you want.

What specifically are you wanting to export to userspace for a LED device
that is not working with the existing LED api in the kernel?

thanks,

greg k-h

jon.schell

unread,
Feb 10, 2011, 1:52:01 PM2/10/11
to Android Linux Kernel Development
1. I'm using an LED driver for testing, it's not relevant to the
issue.
2. Thank you for pointing out that the problem is likely with the
return value. I was going from chapter 14 of LDD3 and didn't check
what the regular char driver function returned back in chapter 3.
3. Do you have an alternate way to pass an array of binary data to a
driver that doesn't have such a capability, like an LED driver? For
an LED driver, this is useful if you wanted to send it a particular
pattern of blinking to run at a faster rate than just setting it off/
on from userspace could do, for example.

On Feb 9, 7:02 pm, Greg KH <gre...@gmail.com> wrote:

Greg KH

unread,
Feb 10, 2011, 2:07:10 PM2/10/11
to android...@googlegroups.com
On Thu, Feb 10, 2011 at 10:52 AM, jon.schell <jon.s...@kyocera.com> wrote:
> 1. I'm using an LED driver for testing, it's not relevant to the
> issue.

Why isn't it relevant? You are using a LED driver, which already
has a well-specified interface for interacting with userspace, why
are you trying to create a new one? That will only break userspace
tools and is not allowed.

> 3. Do you have an alternate way to pass an array of binary data to a
> driver that doesn't have such a capability, like an LED driver?
> For an LED driver, this is useful if you wanted to send it a particular
> pattern of blinking to run at a faster rate than just setting it off/
> on from userspace could do, for example.

Are you sure the existing user/kernel api can't handle this already?
Are you wanting to do PWM with the LED to get it to change to different
values somehow? If so, I think the existing interface can handle this
today, right? Just set the different values in your driver beforehand.

thanks,

greg k-h

jon.schell

unread,
Feb 10, 2011, 2:42:45 PM2/10/11
to Android Linux Kernel Development
Yes, I'm sure it can't handle it. Not a PWM, doing something like:
Turn on for 3 ms, turn off for 5ms, turn on for 1 ms, turn off for 1
ms, turn on for 2 ms, turn off for 8 ms, etc. for any arbitrary
pattern you want to do.

I'm not breaking anything with userspace as it's not a device anyone
will/can access except for my app. And it won't be released into the
code base.

On Feb 10, 11:07 am, Greg KH <gre...@gmail.com> wrote:

Greg KH

unread,
Feb 10, 2011, 2:57:51 PM2/10/11
to android...@googlegroups.com
On Thu, Feb 10, 2011 at 11:42 AM, jon.schell <jon.s...@kyocera.com> wrote:
> Yes, I'm sure it can't handle it.  Not a PWM, doing something like:
> Turn on for 3 ms, turn off for 5ms, turn on for 1 ms, turn off for 1
> ms, turn on for 2 ms, turn off for 8 ms, etc. for any arbitrary
> pattern you want to do.

That type of pattern is easy to do from userspace, please do it that
way and not from within the kernel, that's not the proper place to do
it.

> I'm not breaking anything with userspace as it's not a device anyone
> will/can access except for my app.

You are creating a user/kernel API which is important to get correct.

> And it won't be released into the code base.

What do you mean by this? All kernel code must be published when
the product is shipped.

thanks,

greg k-h

jon.schell

unread,
Feb 10, 2011, 9:17:40 PM2/10/11
to Android Linux Kernel Development
1. I've been reading some other support threads that you answer, and
wow, you are really unimaginative and constrained. Yes, the pattern
can be done from userspace, very slowly. Too slowly to be useful in
fact. When someone asks a question, the best thing to do is answer
the question and don't try to second-guess their motives or say "don't
do it that way" when you don't know why they're doing it that way, or
what the big picture of what they're doing is, which most people
aren't going to be telling you for a simple question.

2. No, I'm not creating a general kernel API, I'm creating something
that only I will be using, for my purpose, so it can be "incorrect" or
"not quite right" if I want. As I said, if you know of some other way
to get a block of binary data into the driver, feel free to suggest
it, but this way works great and will end my headache, so I don't care
if it's proper.

3. Proprietary code is not published.

On Feb 10, 11:57 am, Greg KH <gre...@gmail.com> wrote:

Jon Pry

unread,
Feb 10, 2011, 9:49:33 PM2/10/11
to android...@googlegroups.com
Depending on the pattern of access, you can probably accomplish this with a standard char driver. IOControl's come to mind. More advanced scenarios would probably call for a block driver with memory map support. There's lots of stuff out there on writing a simple char driver, and even the most trivial usually have some IOControl's implemented. Only trouble with howto's is that they tend to be old and writing simple drivers is actually much easier on newer kernel's. Lots of infrastructure for making them in very few lines of code.

Tim Bird

unread,
Feb 10, 2011, 10:04:24 PM2/10/11
to android...@googlegroups.com
On 02/10/2011 06:17 PM, jon.schell wrote:
> 1. I've been reading some other support threads that you answer, and
> wow, you are really unimaginative and constrained. Yes, the pattern
> can be done from userspace, very slowly. Too slowly to be useful in
> fact. When someone asks a question, the best thing to do is answer
> the question and don't try to second-guess their motives or say "don't
> do it that way" when you don't know why they're doing it that way, or
> what the big picture of what they're doing is, which most people
> aren't going to be telling you for a simple question.

The fact that Greg is paying attention to your questions at all is
a sign that he's trying to help. Greg is a very experienced kernel
expert. When he asks a question about what you are doing, it is
so that he can provide the best help possible. When he says
that something can be done another way better, it's based on
what you've described so far in your e-mails.

Very, very often when people inexperienced with the kernel
ask how to do something, they are missing knowledge about
a kernel system that already supports their need. But also
very often, people aren't very forthcoming with what their
exact need really is.

> 2. No, I'm not creating a general kernel API, I'm creating something
> that only I will be using, for my purpose, so it can be "incorrect" or
> "not quite right" if I want. As I said, if you know of some other way
> to get a block of binary data into the driver, feel free to suggest
> it, but this way works great and will end my headache, so I don't care
> if it's proper.

If you don't plan to maintain the code, or push it to mainline,
or release binaries including it to anyone - you are of course
free to do anything you want. If you do plan any of the above, it
could save you time and trouble to listen to Greg.

There are LOTS of ways to move binary data between user space
and the kernel. Which one is appropriate depends on a LOT
of considerations.

> 3. Proprietary code is not published.

If you don't plan to ship the kernel to anyone (for example, if it's
just part of some internal test framework), then that's right.
Otherwise, that's wrong.

-- Tim

> On Feb 10, 11:57�am, Greg KH <gre...@gmail.com> wrote:
>> On Thu, Feb 10, 2011 at 11:42 AM, jon.schell <jon.sch...@kyocera.com> wrote:
>>> Yes, I'm sure it can't handle it. �Not a PWM, doing something like:
>>> Turn on for 3 ms, turn off for 5ms, turn on for 1 ms, turn off for 1
>>> ms, turn on for 2 ms, turn off for 8 ms, etc. for any arbitrary
>>> pattern you want to do.
>>
>> That type of pattern is easy to do from userspace, please do it that
>> way and not from within the kernel, that's not the proper place to do
>> it.
>>
>>> I'm not breaking anything with userspace as it's not a device anyone
>>> will/can access except for my app.
>>
>> You are creating a user/kernel API which is important to get correct.
>>
>>> �And it won't be released into the code base.
>>
>> What do you mean by this? �All kernel code must be published when
>> the product is shipped.
>>
>> thanks,
>>
>> greg k-h
>


--
=============================
Tim Bird
Architecture Group Chair, CE Linux Forum
Senior Staff Engineer, Sony Network Entertainment
=============================

Greg KH

unread,
Feb 10, 2011, 10:05:00 PM2/10/11
to android...@googlegroups.com
On Thu, Feb 10, 2011 at 6:17 PM, jon.schell <jon.s...@kyocera.com> wrote:
> 1. I've been reading some other support threads that you answer, and
> wow, you are really unimaginative and constrained.  Yes, the pattern
> can be done from userspace, very slowly.  Too slowly to be useful in
> fact.  When someone asks a question, the best thing to do is answer
> the question and don't try to second-guess their motives or say "don't
> do it that way" when you don't know why they're doing it that way, or
> what the big picture of what they're doing is, which most people
> aren't going to be telling you for a simple question.

Ok, you know best.

> 2. No, I'm not creating a general kernel API, I'm creating something
> that only I will be using, for my purpose, so it can be "incorrect" or
> "not quite right" if I want.  As I said, if you know of some other way
> to get a block of binary data into the driver, feel free to suggest
> it, but this way works great and will end my headache, so I don't care
> if it's proper.

There are lots of ways to do this. Using sysfs is not the way.

> 3. Proprietary code is not published.

Then don't use open source and ask for questions on public mailing
lists.

good luck,

greg k-h

jon.schell

unread,
Feb 10, 2011, 11:10:45 PM2/10/11
to Android Linux Kernel Development
For this issue, yes, I do. I asked a simple question, that had a
simple answer, which I appreciate. I didn't need the rest of the
lecture about how I shouldn't be doing what I'm doing for whatever
reason.

2. "Lots of ways to do this" isn't a useful comment. Using sysfs fits
my model as far as I can tell. If you have a suggestion for a
specific way that fits my model better, feel free to offer it.

3. That's not a rational comment. Android and Linux are open source.
My question is about using those in particular. Just because it
interfaces to something proprietary does not make it an inappropriate
topic for this mailing list.

On Feb 10, 7:05 pm, Greg KH <gre...@gmail.com> wrote:

jon.schell

unread,
Feb 10, 2011, 11:15:06 PM2/10/11
to Android Linux Kernel Development
It was a simple question, it was obviously a 10-second answer, which I
appreciate getting. The rest of his attitude is what this last
comment was about. Yes, often the person asking the question can be
someone who doesn't know better, but there's no reason to assume
that. Question it first at least, don't just assume. And if they
aren't forthcoming with what their real need is, it may be because
they can't be (because of confidentiality). Regardless, just saying
"don't do this" doesn't help anything.
> =============================- Hide quoted text -
>
> - Show quoted text -

jon.schell

unread,
Feb 10, 2011, 11:18:17 PM2/10/11
to Android Linux Kernel Development
What's the difference between writing a char driver from scratch that
has a single attribute called "brightness" and copying the red led
driver and calling it the shoe led so that it inherits all the led
class attributes (of which there's only one, which is "brightness")?

Jon Pry

unread,
Feb 11, 2011, 10:08:04 PM2/11/11
to android...@googlegroups.com
char drivers don't have attributes, they have a single bidirectional data stream and iocontrol's.
the stuff in sysfs is similar to a char driver from the users perspective, except for iocontrol,
which is what i think you want.

Reply all
Reply to author
Forward
0 new messages