Re: [PATCH 0/3] RFC: The beginning of a proper driver-api book
Am 23.08.2016 um 16:43 schrieb Mauro Carvalho Chehab : > Em Mon, 22 Aug 2016 14:57:40 -0600 > Jonathan Corbet escreveu: > >> This short series convers device-drivers.tmpl into the RST format, splits >> it up, and sets up the result under Documentation/driver-api/. For added >> fun, I've taken one top-level file (hsi.txt) and folded it into the >> document as a way of showing the direction I'm thinking I would like things >> to go. There is plenty more of this sort of work that could be done, to >> say the least - this is just a beginning! >> >> The formatted results can be seen at: >> >> http://static.lwn.net/kerneldoc/driver-api/index.html > > Thanks for doing that! IMHO, the conversion of this book is indeed > one of the first things to be done. >> As part of the long-term task to turn Documentation/ into less of a horror >> movie, I'd like to collect documentation of the driver-specific API here. Hi, here are my 2cent, about the *generic* content from the kernel-doc directive: .. kernel-doc:: kernel/sched/core.c :export: IMHO directives like the one above are to *generic*. If I read this directive I would expect, that all exported symbols are documented. But this is a false estimation! In my POC I use a more restrictive kernel-doc parser (https://github.com/return42/linuxdoc). For the directive above the parser logs, that some of the exported symbols are not found / not documented: $ kernel-doc --quiet --list-exports kernel/sched/core.c [exported undocumented ] set_cpus_allowed_ptr [exported undocumented ] kick_process [exported function ] wake_up_process [exported undocumented ] preempt_notifier_inc [exported undocumented ] preempt_notifier_dec [exported function ] preempt_notifier_register [exported function ] preempt_notifier_unregister [exported undocumented ] single_task_running [exported undocumented ] preempt_count_add [exported undocumented ] preempt_count_sub [exported undocumented ] schedule [exported undocumented ] preempt_schedule [exported function ] preempt_schedule_notrace [exported undocumented ] default_wake_function [exported undocumented ] set_user_nice [exported function ] sched_setscheduler [exported undocumented ] sched_setattr [exported function ] sched_setscheduler_nocheck [exported undocumented ] _cond_resched [exported undocumented ] __cond_resched_lock [exported undocumented ] __cond_resched_softirq [exported function ] yield [exported function ] yield_to [exported undocumented ] io_schedule_timeout [exported undocumented ] __might_sleep [exported undocumented ] ___might_sleep The driver-api is full of *generic* content and IMHO it is not really clear what would be a part of the resulting documentation. To illustrate, you can take a look on the (old) *automatic* conversion of mine at: http://return42.github.io/sphkerneldoc/books/device-drivers/index.html There you see a list of 'Oops: Document generation inconsistency.' This kind of missing documentation grows up with changes to the source files while there are no errors reported. What I mean: in most use cases it is better to be explicit and name the functions, structs or whatever which should be a part of the documentation. e.g.:: .. kernel-doc:: kernel/sched/core.c :functions: wake_up_process yield ... By being explicit, the kernel-doc parser has a chance to identify requested but missing documentation and log related error messages. Summarized: - *explicit* is better than implicit. - the *generic* part of kernel-doc parser should more restrictive These are my thoughts, even if we do nothing to handle it, we should aware about this. -- Markus -- > >> Arguably gpu/ and the media API stuff should eventually move here, though >> we can discuss the color of that particular shed some other day. > > Yeah, let's finish converting everything to rst, and then see what would > be the best way to organize it. > > I have a split feelings with regards to media kAPI and driver-specific > stuff if it is better to keep it at the device-drivers book, or at a > single media book (with currently has 1031 pages on PDF output), > but let's postpone such discussion. > > I'm afraid that we'll end needing to split it more than what would be > desired, in order to fix some issues with the PDF output > part/chapter/section numbering. Unfortunately, ReST markup is too poor > to describe multi-part books, and doesn't have anything to describe > multi-book projects. > >> Meanwhile, I'd appreciate comments on the general idea. > > I like the general idea of breaking device-drivers.tmpl into multiple > files. IMHO, that will make easier to maintain it long term. > > After looking on all 3 patches, they all look good, as a concept. > > Yet, IMHO, there are a few things to be fixed before merging it. See > my comments to patch 2/3. > > Regards, > Mauro > >> >> Cc: Jani Nikula >> Cc: Mauro Carvalho Chehab >> Cc: Sebas
Re: [PATCH 0/3] RFC: The beginning of a proper driver-api book
Em Fri, 26 Aug 2016 11:34:38 +0200 Markus Heiser escreveu: > Am 23.08.2016 um 16:43 schrieb Mauro Carvalho Chehab > : > > > Em Mon, 22 Aug 2016 14:57:40 -0600 > > Jonathan Corbet escreveu: > > > >> This short series convers device-drivers.tmpl into the RST format, splits > >> it up, and sets up the result under Documentation/driver-api/. For added > >> fun, I've taken one top-level file (hsi.txt) and folded it into the > >> document as a way of showing the direction I'm thinking I would like things > >> to go. There is plenty more of this sort of work that could be done, to > >> say the least - this is just a beginning! > >> > >> The formatted results can be seen at: > >> > >> http://static.lwn.net/kerneldoc/driver-api/index.html > > > > Thanks for doing that! IMHO, the conversion of this book is indeed > > one of the first things to be done. > > >> As part of the long-term task to turn Documentation/ into less of a horror > >> movie, I'd like to collect documentation of the driver-specific API here. > > Hi, > > here are my 2cent, about the *generic* content from the kernel-doc > directive: > > .. kernel-doc:: kernel/sched/core.c > :export: > > IMHO directives like the one above are to *generic*. If I read this directive > I would expect, that all exported symbols are documented. But this is a false > estimation! > > In my POC I use a more restrictive kernel-doc parser > (https://github.com/return42/linuxdoc). For the directive above the parser > logs, that some of the exported symbols are not found / not documented: > > $ kernel-doc --quiet --list-exports kernel/sched/core.c > [exported undocumented ] set_cpus_allowed_ptr > [exported undocumented ] kick_process > [exported function ] wake_up_process > [exported undocumented ] preempt_notifier_inc > [exported undocumented ] preempt_notifier_dec > [exported function ] preempt_notifier_register > [exported function ] preempt_notifier_unregister > [exported undocumented ] single_task_running > [exported undocumented ] preempt_count_add > [exported undocumented ] preempt_count_sub > [exported undocumented ] schedule > [exported undocumented ] preempt_schedule > [exported function ] preempt_schedule_notrace > [exported undocumented ] default_wake_function > [exported undocumented ] set_user_nice > [exported function ] sched_setscheduler > [exported undocumented ] sched_setattr > [exported function ] sched_setscheduler_nocheck > [exported undocumented ] _cond_resched > [exported undocumented ] __cond_resched_lock > [exported undocumented ] __cond_resched_softirq > [exported function ] yield > [exported function ] yield_to > [exported undocumented ] io_schedule_timeout > [exported undocumented ] __might_sleep > [exported undocumented ] ___might_sleep > > > The driver-api is full of *generic* content and IMHO it is not really clear > what would be a part of the resulting documentation. To illustrate, you > can take a look on the (old) *automatic* conversion of mine at: > > http://return42.github.io/sphkerneldoc/books/device-drivers/index.html > > There you see a list of 'Oops: Document generation inconsistency.' > This kind of missing documentation grows up with changes to > the source files while there are no errors reported. > > What I mean: in most use cases it is better to be explicit and name the > functions, structs or whatever which should be a part of the documentation. > e.g.:: > > .. kernel-doc:: kernel/sched/core.c > :functions: wake_up_process yield ... > > By being explicit, the kernel-doc parser has a chance to identify requested > but missing documentation and log related error messages. > > Summarized: > > - *explicit* is better than implicit. > - the *generic* part of kernel-doc parser should more restrictive > > These are my thoughts, even if we do nothing to handle it, we > should aware about this. I actually prefer the opposite: - on a *.c file, it should assume that *all* exported symbols should be documented (either at the C code itself or at a header file); - on a *.h file, it should assume that *all* structs, enums, typedefs, function prototypes and static inline functions should be documented. As I stated before, we should also add a way to document defines. Assuming that we add such way, for defines, it should implicitly ignore the ones used inside the header to enable/disable part of its contents, like: #define _FOO_H_ #ifndef _FOO_H_ #endif Then, add an option to allow explicitly ignoring symbols. The lack of documentation for a symbol that matches the above criteria and isn't explicitly ignored should be warned, as this is a documentation gap that should be fixed. Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.ker
Re: [PATCH 0/3] RFC: The beginning of a proper driver-api book
On Fri, 26 Aug 2016, Mauro Carvalho Chehab wrote: > Em Fri, 26 Aug 2016 11:34:38 +0200 > Markus Heiser escreveu: > >> Am 23.08.2016 um 16:43 schrieb Mauro Carvalho Chehab >> : >> >> > Em Mon, 22 Aug 2016 14:57:40 -0600 >> > Jonathan Corbet escreveu: >> > >> >> This short series convers device-drivers.tmpl into the RST format, splits >> >> it up, and sets up the result under Documentation/driver-api/. For added >> >> fun, I've taken one top-level file (hsi.txt) and folded it into the >> >> document as a way of showing the direction I'm thinking I would like >> >> things >> >> to go. There is plenty more of this sort of work that could be done, to >> >> say the least - this is just a beginning! >> >> >> >> The formatted results can be seen at: >> >> >> >> http://static.lwn.net/kerneldoc/driver-api/index.html >> > >> > Thanks for doing that! IMHO, the conversion of this book is indeed >> > one of the first things to be done. >> >> >> As part of the long-term task to turn Documentation/ into less of a horror >> >> movie, I'd like to collect documentation of the driver-specific API here. >> >> >> >> Hi, >> >> here are my 2cent, about the *generic* content from the kernel-doc >> directive: >> >> .. kernel-doc:: kernel/sched/core.c >> :export: >> >> IMHO directives like the one above are to *generic*. If I read this directive >> I would expect, that all exported symbols are documented. But this is a false >> estimation! >> >> In my POC I use a more restrictive kernel-doc parser >> (https://github.com/return42/linuxdoc). For the directive above the parser >> logs, that some of the exported symbols are not found / not documented: >> >> $ kernel-doc --quiet --list-exports kernel/sched/core.c >> [exported undocumented ] set_cpus_allowed_ptr >> [exported undocumented ] kick_process >> [exported function ] wake_up_process >> [exported undocumented ] preempt_notifier_inc >> [exported undocumented ] preempt_notifier_dec >> [exported function ] preempt_notifier_register >> [exported function ] preempt_notifier_unregister >> [exported undocumented ] single_task_running >> [exported undocumented ] preempt_count_add >> [exported undocumented ] preempt_count_sub >> [exported undocumented ] schedule >> [exported undocumented ] preempt_schedule >> [exported function ] preempt_schedule_notrace >> [exported undocumented ] default_wake_function >> [exported undocumented ] set_user_nice >> [exported function ] sched_setscheduler >> [exported undocumented ] sched_setattr >> [exported function ] sched_setscheduler_nocheck >> [exported undocumented ] _cond_resched >> [exported undocumented ] __cond_resched_lock >> [exported undocumented ] __cond_resched_softirq >> [exported function ] yield >> [exported function ] yield_to >> [exported undocumented ] io_schedule_timeout >> [exported undocumented ] __might_sleep >> [exported undocumented ] ___might_sleep >> >> >> The driver-api is full of *generic* content and IMHO it is not really clear >> what would be a part of the resulting documentation. To illustrate, you >> can take a look on the (old) *automatic* conversion of mine at: >> >> http://return42.github.io/sphkerneldoc/books/device-drivers/index.html >> >> There you see a list of 'Oops: Document generation inconsistency.' >> This kind of missing documentation grows up with changes to >> the source files while there are no errors reported. >> >> What I mean: in most use cases it is better to be explicit and name the >> functions, structs or whatever which should be a part of the documentation. >> e.g.:: >> >> .. kernel-doc:: kernel/sched/core.c >> :functions: wake_up_process yield ... >> >> By being explicit, the kernel-doc parser has a chance to identify requested >> but missing documentation and log related error messages. >> >> Summarized: >> >> - *explicit* is better than implicit. >> - the *generic* part of kernel-doc parser should more restrictive >> >> These are my thoughts, even if we do nothing to handle it, we >> should aware about this. > > I actually prefer the opposite: Ditto. Jani. > > - on a *.c file, it should assume that *all* exported symbols should be > documented (either at the C code itself or at a header file); > > - on a *.h file, it should assume that *all* structs, enums, typedefs, > function prototypes and static inline functions should be documented. > As I stated before, we should also add a way to document defines. > Assuming that we add such way, for defines, it should implicitly > ignore the ones used inside the header to enable/disable part of > its contents, like: > #define _FOO_H_ > #ifndef _FOO_H_ > > #endif > > Then, add an option to allow explicitly ignoring symbols. The lack > of documentation for a symbol that matches the above criteria and > isn't explicitly ignored should be warned, as this is a documentati
[PATCH] docs: make kernel-doc handle varargs properly
As far as I can tell, the handling of "..." arguments has never worked
right, so any documentation provided was ignored in favor of "variable
arguments." This makes kernel-doc handle "@...:" as documented. It does
*not* fix spots in kerneldoc comments that don't follow that convention,
but they are no more broken than before.
Signed-off-by: Jonathan Corbet
---
scripts/kernel-doc | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index c681e8f0ecc2..e6c52ab938fd 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -414,7 +414,7 @@ my $doc_com_body = '\s*\* ?';
my $doc_decl = $doc_com . '(\w+)';
# @params and a strictly limited set of supported section names
my $doc_sect = $doc_com .
-'\s*(\@\w+|description|context|returns?|notes?|examples?)\s*:(.*)';
+'\s*(\@[.\w]+|description|context|returns?|notes?|examples?)\s*:(.*)';
my $doc_content = $doc_com_body . '(.*)';
my $doc_block = $doc_com . 'DOC:\s*(.*)?';
my $doc_inline_start = '^\s*/\*\*\s*$';
@@ -2340,6 +2340,7 @@ sub push_parameter($$$) {
if ($type eq "" && $param =~ /\.\.\.$/)
{
+ $param = "...";
if (!defined $parameterdescs{$param} || $parameterdescs{$param} eq
"") {
$parameterdescs{$param} = "variable arguments";
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: move oneshot trigger attributes documentation to ABI
Hi Rafał, Thanks for the patch. We could possibly correct one linguistic issue whilst we are at it. Please take a look below. On 08/25/2016 11:38 AM, Rafał Miłecki wrote: From: Rafał Miłecki Documentation of sysfs interface should be in ABI in the first place. This moves relevant part of documentation and mentions where to look for it. Signed-off-by: Rafał Miłecki --- Documentation/ABI/testing/sysfs-class-led | 3 +- .../ABI/testing/sysfs-class-led-trigger-oneshot| 37 ++ Documentation/leds/ledtrig-oneshot.txt | 20 ++-- 3 files changed, 41 insertions(+), 19 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-oneshot diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led index 3646ec8..86ace28 100644 --- a/Documentation/ABI/testing/sysfs-class-led +++ b/Documentation/ABI/testing/sysfs-class-led @@ -24,7 +24,8 @@ Description: of led events. You can change triggers in a similar manner to the way an IO scheduler is chosen. Trigger specific parameters can appear in - /sys/class/leds/ once a given trigger is selected. + /sys/class/leds/ once a given trigger is selected. For + their documentation see sysfs-class-led-trigger-*. What: /sys/class/leds//inverted Date: January 2011 diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-oneshot b/Documentation/ABI/testing/sysfs-class-led-trigger-oneshot new file mode 100644 index 000..401cbe6 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-oneshot @@ -0,0 +1,37 @@ +What: /sys/class/leds//delay_on +Date: Jun 2012 +KernelVersion: 3.6 +Contact: linux-l...@vger.kernel.org +Description: + Specifies for how many milliseconds the LED has to stay at + LED_FULL brightness after it has been armed. + Default to 100 ms. s/Default/Defaults/ + + +What: /sys/class/leds//delay_off +Date: Jun 2012 +KernelVersion: 3.6 +Contact: linux-l...@vger.kernel.org +Description: + Specifies for how many milliseconds the LED has to stay at + LED_OFF brightness after it has been armed. + Default to 100 ms. s/Default/Defaults/ + +What: /sys/class/leds//invert +Date: Jun 2012 +KernelVersion: 3.6 +Contact: linux-l...@vger.kernel.org +Description: + Reverse the blink logic. If set to 0 (default) blink on for + delay_on ms, then blink off for delay_off ms, leaving the LED + normally off. If set to 1, blink off for delay_off ms, then + blink on for delay_on ms, leaving the LED normally on. + Setting this value also immediately change the LED state. + +What: /sys/class/leds//shot +Date: Jun 2012 +KernelVersion: 3.6 +Contact: linux-l...@vger.kernel.org +Description: + Write any non-empty string to signal an events, this starts a + blink sequence if not already running. diff --git a/Documentation/leds/ledtrig-oneshot.txt b/Documentation/leds/ledtrig-oneshot.txt index 07cd1fa..fe57474 100644 --- a/Documentation/leds/ledtrig-oneshot.txt +++ b/Documentation/leds/ledtrig-oneshot.txt @@ -21,24 +21,8 @@ below: echo oneshot > trigger -This adds the following sysfs attributes to the LED: - - delay_on - specifies for how many milliseconds the LED has to stay at - LED_FULL brightness after it has been armed. - Default to 100 ms. - - delay_off - specifies for how many milliseconds the LED has to stay at - LED_OFF brightness after it has been armed. - Default to 100 ms. - - invert - reverse the blink logic. If set to 0 (default) blink on for delay_on - ms, then blink off for delay_off ms, leaving the LED normally off. If - set to 1, blink off for delay_off ms, then blink on for delay_on ms, - leaving the LED normally on. - Setting this value also immediately change the LED state. - - shot - write any non-empty string to signal an events, this starts a blink - sequence if not already running. +This adds sysfs attributes to the LED that are documented in: +Documentation/ABI/testing/sysfs-class-led-trigger-oneshot Example use-case: network devices, initialization: -- Best regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] Documentation: move oneshot trigger attributes documentation to ABI
From: Rafał Miłecki Documentation of sysfs interface should be in ABI in the first place. This moves relevant part of documentation and mentions where to look for it. Fix trivial typos whilst we are at it. Signed-off-by: Rafał Miłecki --- V2: s/Default/Defaults/ s/ / / s/change/changes/ --- Documentation/ABI/testing/sysfs-class-led | 3 +- .../ABI/testing/sysfs-class-led-trigger-oneshot| 36 ++ Documentation/leds/ledtrig-oneshot.txt | 20 ++-- 3 files changed, 40 insertions(+), 19 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-oneshot diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led index 3646ec8..86ace28 100644 --- a/Documentation/ABI/testing/sysfs-class-led +++ b/Documentation/ABI/testing/sysfs-class-led @@ -24,7 +24,8 @@ Description: of led events. You can change triggers in a similar manner to the way an IO scheduler is chosen. Trigger specific parameters can appear in - /sys/class/leds/ once a given trigger is selected. + /sys/class/leds/ once a given trigger is selected. For + their documentation see sysfs-class-led-trigger-*. What: /sys/class/leds//inverted Date: January 2011 diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-oneshot b/Documentation/ABI/testing/sysfs-class-led-trigger-oneshot new file mode 100644 index 000..378a3a4 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-oneshot @@ -0,0 +1,36 @@ +What: /sys/class/leds//delay_on +Date: Jun 2012 +KernelVersion: 3.6 +Contact: linux-l...@vger.kernel.org +Description: + Specifies for how many milliseconds the LED has to stay at + LED_FULL brightness after it has been armed. + Defaults to 100 ms. + +What: /sys/class/leds//delay_off +Date: Jun 2012 +KernelVersion: 3.6 +Contact: linux-l...@vger.kernel.org +Description: + Specifies for how many milliseconds the LED has to stay at + LED_OFF brightness after it has been armed. + Defaults to 100 ms. + +What: /sys/class/leds//invert +Date: Jun 2012 +KernelVersion: 3.6 +Contact: linux-l...@vger.kernel.org +Description: + Reverse the blink logic. If set to 0 (default) blink on for + delay_on ms, then blink off for delay_off ms, leaving the LED + normally off. If set to 1, blink off for delay_off ms, then + blink on for delay_on ms, leaving the LED normally on. + Setting this value also immediately changes the LED state. + +What: /sys/class/leds//shot +Date: Jun 2012 +KernelVersion: 3.6 +Contact: linux-l...@vger.kernel.org +Description: + Write any non-empty string to signal an events, this starts a + blink sequence if not already running. diff --git a/Documentation/leds/ledtrig-oneshot.txt b/Documentation/leds/ledtrig-oneshot.txt index 07cd1fa..fe57474 100644 --- a/Documentation/leds/ledtrig-oneshot.txt +++ b/Documentation/leds/ledtrig-oneshot.txt @@ -21,24 +21,8 @@ below: echo oneshot > trigger -This adds the following sysfs attributes to the LED: - - delay_on - specifies for how many milliseconds the LED has to stay at - LED_FULL brightness after it has been armed. - Default to 100 ms. - - delay_off - specifies for how many milliseconds the LED has to stay at - LED_OFF brightness after it has been armed. - Default to 100 ms. - - invert - reverse the blink logic. If set to 0 (default) blink on for delay_on - ms, then blink off for delay_off ms, leaving the LED normally off. If - set to 1, blink off for delay_off ms, then blink on for delay_on ms, - leaving the LED normally on. - Setting this value also immediately change the LED state. - - shot - write any non-empty string to signal an events, this starts a blink - sequence if not already running. +This adds sysfs attributes to the LED that are documented in: +Documentation/ABI/testing/sysfs-class-led-trigger-oneshot Example use-case: network devices, initialization: -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4] leds: trigger: Introduce an USB port trigger
On 25 August 2016 at 14:49, Greg KH wrote:
> On Thu, Aug 25, 2016 at 10:03:52AM +0200, Rafał Miłecki wrote:
>> +static void usbport_trig_activate(struct led_classdev *led_cdev)
>> +{
>> + struct usbport_trig_data *usbport_data;
>> + int err;
>> +
>> + usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
>> + if (!usbport_data)
>> + return;
>> + usbport_data->led_cdev = led_cdev;
>> +
>> + /* Storing ports */
>> + INIT_LIST_HEAD(&usbport_data->ports);
>> + usbport_data->ports_dir = kobject_create_and_add("ports",
>> + &led_cdev->dev->kobj);
>
> If you _ever_ find yourself in a driver having to use kobject calls,
> it's a HUGE hint that you are doing something wrong.
>
> Hint, you are doing this wrong :)
>
> Use an attribute group if you need a subdirectory in sysfs, using a
> "raw" kobject like this hides it from all userspace tools and so no
> userspace program can see it (hint, try using libudev to access these
> files attached to the device...)
Oops. Thanks for pointing groups to me. I was looking at sysfs.h
earlier but I didn't realize group can be a subdirectory. I can see
these sysfs_create_group(s) and friends now, thanks.
>> + if (!usbport_data->ports_dir)
>> + goto err_free;
>> +
>> + /* API for ports management */
>> + err = device_create_file(led_cdev->dev, &dev_attr_new_port);
>> + if (err)
>> + goto err_put_ports;
>> + err = device_create_file(led_cdev->dev, &dev_attr_remove_port);
>> + if (err)
>> + goto err_remove_new_port;
>
> Doesn't this race with userspace and fail? Shouldn't the led core be
> creating your leds for you?
These questions aren't clear to me. What kind of race? Doing
echo usbport > trigger
results in trigger core calling usbport_trig_activate. We create new
attributes and then we return.
I'm also not creating any leds there. This already has to be LED
available if you want to assign some trigger to it.
>> +
>> + /* Notifications */
>> + usbport_data->nb.notifier_call = usbport_trig_notify,
>> + led_cdev->trigger_data = usbport_data;
>> + usb_register_notify(&usbport_data->nb);
>
> Don't abuse the USB notifier chain with stuff like this please, is that
> really necessary? Why can't your hardware just tell you what USB ports
> are in use "out of band"?
I totally don't understand this part. This LED trigger is supposed to
react to USB devices appearing at specified ports. How else can I know
there is a new USB device if not by notifications?
I'm wondering if we are on the same page. Could it be my idea of this
LED trigger is not clear at all? Could you take a look at commit
message again, please? Mostly this part:
> This commit adds a new trigger responsible for turning on LED when USB
> device gets connected to the specified USB port. This can can useful for
> various home routers that have USB port(s) and a proper LED telling user
> a device is connected.
Can I add something more to make it clear what this trigger is responsible for?
>> +
>> + led_cdev->activated = true;
>> + return;
>> +
>> +err_remove_new_port:
>> + device_remove_file(led_cdev->dev, &dev_attr_new_port);
>> +err_put_ports:
>> + kobject_put(usbport_data->ports_dir);
>> +err_free:
>> + kfree(usbport_data);
>> +}
>
> And again, why is this USB specific? Why can't you use this same
> userspace api and codebase for PCI ports? For a sdcard port? For a
> thunderbolt port?
I'm leaving this one unanswered as discussion on this continued in
V3.5 thread below my reply:
On 25 August 2016 at 07:14, Rafał Miłecki wrote:
> Good question. I would like to extend this USB port trigger in the
> future by reacting to USB activity. This involves playing with URBs
> and I believe that at that point it'd be getting too much USB specific
> to /rule them all/.
--
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger
On 25 August 2016 at 20:48, Jacek Anaszewski wrote: > On 08/25/2016 04:30 PM, Alan Stern wrote: >> >> On Thu, 25 Aug 2016, Jacek Anaszewski wrote: >> >>> I'd see it as follows: >>> >>> #cat available_ports >>> #1-1 1-2 2-1 >>> >>> #echo "1-1" > new_port >>> >>> #cat observed_ports >>> #1-1 >>> >>> #echo "2-1" > new_port >>> >>> #cat observed_ports >>> #1-1 2-1 >>> >>> We've already had few discussions about the sysfs designs trying >>> to break the one-value-per-file rule for LED class device, and >>> there was always strong resistance against. >> >> >> This scheme has multiple values in both the available_ports and >> observed_ports files. :-( Not that I have any better suggestions... > > > Right, I forgot to add a note here, that this follows space > separated list pattern similarly as in case of triggers attribute. > Of course other suggestions are welcome. So ppl have doubts about multiple values in a single sysfs file (whatever we call it: "ports" or "observed_ports"). Greg clearly said: > sysfs is "one value per file", here you are listing a bunch of things in > one sysfs file. Please don't do that. What about my idea of using "ports" subdirectory and having each port as separated file inside that subdir? I think there are two ways of doing this: 1) Having "ports" subdir with 0x chmod files, one per each port specified as observable In this solution we need "new_port" and "remove_port" that can be used for management of observable ports. I think Jacek wasn't happy with this chmod and he believes Greg meant R/W files. 2) Having "ports" subdir with RW files, one per each existing physical port In this situation we don't need "new_port" or "remove_port". If we want port to be observable we just do: echo 1 > 1-1 Implementing this solution needs reading more details from USB subsystem. Do you find any of solutions with "ports" subdir better than dealing with new-line/space separated values in a single sysfs file? -- Rafał -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] docs: make kernel-doc handle varargs properly
Hi Jonathan, [auto build test WARNING on linus/master] [also build test WARNING on v4.8-rc3 next-20160825] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information] url: https://github.com/0day-ci/linux/commits/Jonathan-Corbet/docs-make-kernel-doc-handle-varargs-properly/20160826-213158 reproduce: make htmldocs All warnings (new ones prefixed by >>): include/sound/compress_driver.h:162: warning: No description found for parameter 'id[64]' include/sound/compress_driver.h:162: warning: No description found for parameter 'proc_root' include/sound/compress_driver.h:162: warning: No description found for parameter 'proc_info_entry' >> include/sound/core.h:323: warning: No description found for parameter '...' include/sound/core.h:334: warning: No description found for parameter '...' include/sound/core.h:387: warning: No description found for parameter '...' -- include/linux/init.h:1: warning: no structured comments found kernel/sched/core.c:2082: warning: No description found for parameter 'cookie' >> include/linux/workqueue.h:391: warning: No description found for parameter >> '...' >> include/linux/workqueue.h:391: warning: Excess function parameter 'args' >> description in 'alloc_workqueue' include/linux/workqueue.h:412: warning: No description found for parameter '...' >> include/linux/workqueue.h:412: warning: Excess function parameter 'args' >> description in 'alloc_ordered_workqueue' kernel/sys.c:1: warning: no structured comments found include/linux/fence.h:85: warning: Excess struct/union/enum/typedef member 'child_list' description in 'fence' include/linux/fence.h:85: warning: Excess struct/union/enum/typedef member 'active_list' description in 'fence' drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found drivers/dma-buf/fence-array.c:124: warning: No description found for parameter 'signal_on_any' >> include/sound/core.h:323: warning: No description found for parameter '...' include/sound/core.h:334: warning: No description found for parameter '...' include/sound/core.h:387: warning: No description found for parameter '...' vim +323 include/sound/core.h b9075fa96 Joe Perches2011-10-31 307 __printf(4, 5) 36ce99c1d Takashi Iwai 2009-08-27 308 void __snd_printk(unsigned int level, const char *file, int line, b9075fa96 Joe Perches2011-10-31 309 const char *format, ...); 36ce99c1d Takashi Iwai 2009-08-27 310 #else 36ce99c1d Takashi Iwai 2009-08-27 311 #define __snd_printk(level, file, line, format, args...) \ cf0baf16c Takashi Iwai 2009-08-28 312printk(format, ##args) 36ce99c1d Takashi Iwai 2009-08-27 313 #endif 36ce99c1d Takashi Iwai 2009-08-27 314 ^1da177e4 Linus Torvalds 2005-04-16 315 /** ^1da177e4 Linus Torvalds 2005-04-16 316 * snd_printk - printk wrapper ^1da177e4 Linus Torvalds 2005-04-16 317 * @fmt: format string ^1da177e4 Linus Torvalds 2005-04-16 318 * f66fcedc8 Takashi Iwai 2008-11-07 319 * Works like printk() but prints the file and the line of the caller ^1da177e4 Linus Torvalds 2005-04-16 320 * when configured with CONFIG_SND_VERBOSE_PRINTK. ^1da177e4 Linus Torvalds 2005-04-16 321 */ ^1da177e4 Linus Torvalds 2005-04-16 322 #define snd_printk(fmt, args...) \ 36ce99c1d Takashi Iwai 2009-08-27 @323__snd_printk(0, __FILE__, __LINE__, fmt, ##args) ^1da177e4 Linus Torvalds 2005-04-16 324 ^1da177e4 Linus Torvalds 2005-04-16 325 #ifdef CONFIG_SND_DEBUG ^1da177e4 Linus Torvalds 2005-04-16 326 /** ^1da177e4 Linus Torvalds 2005-04-16 327 * snd_printd - debug printk 0863afb32 Martin Waitz 2006-01-09 328 * @fmt: format string ^1da177e4 Linus Torvalds 2005-04-16 329 * edab938e6 Pavel Machek 2008-04-01 330 * Works like snd_printk() for debugging purposes. ^1da177e4 Linus Torvalds 2005-04-16 331 * Ignored when CONFIG_SND_DEBUG is not set. :: The code at line 323 was first introduced by commit :: 36ce99c1dcab2978fc1900f19b431adedd8f99f6 ALSA: Add debug module option :: TO: Takashi Iwai :: CC: Takashi Iwai --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger
On Thu 2016-08-25 11:04:38, Jacek Anaszewski wrote: > On 08/25/2016 10:29 AM, Rafał Miłecki wrote: > >On 25 August 2016 at 10:03, Jacek Anaszewski > >wrote: > >>On 08/24/2016 07:52 PM, Rafał Miłecki wrote: > >>> > >>>From: Rafał Miłecki > >>> > >>>This commit adds a new trigger responsible for turning on LED when USB > >>>device gets connected to the specified USB port. This can can useful for > >>>various home routers that have USB port(s) and a proper LED telling user > >>>a device is connected. > >>> > >>>The trigger gets its documentation file but basically it just requires > >>>specifying USB port in a Linux format (e.g. echo 1-1 > new_port). > >>> > >>>During work on this trigger there was a plan to add DT bindings for it, > >>>but there wasn't an agreement on the format yet. This can be worked on > >>>later, a sysfs interface is needed anyway for platforms not using DT. > >>> > >>>Signed-off-by: Rafał Miłecki > >>>--- > >>>V2: Trying to add DT support, idea postponed as it will take more time > >>>to discuss the bindings. > >>>V3: Fix typos in commit and Documentation (thanks Jacek!) > >>>Use "ports" sysfs file for adding and removing USB ports (thx Jacek) > >>>Check if there is USB device connected after adding new USB port > >>>Fix memory leak or two > >>>V3.5: Fix e-mail address (thanks Matthias) > >>> Simplify conditions in usbport_trig_notify (thx Matthias) > >>> Make "ports" a subdirectory with file per port, to match one value > >>> per file sysfs rule (thanks Greg) > >>> As "ports" couldn't be used for adding and removing ports anymore, > >>> there are now "new_port" and "remove_port". Having them makes this > >>> API also common with e.g. pci and usb buses. > >> > >> > >>Now writing new_port with "1-1" produces a file named "1-1" in the > >>ports directory with 000 permissions. I think that what Greg had > >>on mind by referring to "one value per file" rule was a set of > >>files representing ports, like "1-1 1-2 2-1", and each file should be > >>readable/writeable. > >> > >>For instance "echo 1 > 1-1" would enable the trigger for the port 1-1 > >>and "echo 0 > 1-1" would disable it. The problem is that we don't know > >>the number of required ports at compilation time and the sysfs > >>attributes would have to be dynamically created on driver instantiation. > >>What is more, as the USB ports can dynamically appear/disappear in the > >>system, the files would have to be created/removed accordingly during > >>LED class device lifetime, which is not the best design for the sysfs > >>interface I think. Could we add a flag to the USB port, instead? That way, USB code would take care of creating the enable file in its own directory. Is per-port control needed? Would just global control be sufficient? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger
On Thu 2016-08-25 20:48:04, Jacek Anaszewski wrote: > On 08/25/2016 04:30 PM, Alan Stern wrote: > >On Thu, 25 Aug 2016, Jacek Anaszewski wrote: > > > >>I'd see it as follows: > >> > >>#cat available_ports > >>#1-1 1-2 2-1 > >> > >>#echo "1-1" > new_port > >> > >>#cat observed_ports > >>#1-1 > >> > >>#echo "2-1" > new_port > >> > >>#cat observed_ports > >>#1-1 2-1 > >> > >>We've already had few discussions about the sysfs designs trying > >>to break the one-value-per-file rule for LED class device, and > >>there was always strong resistance against. > > > >This scheme has multiple values in both the available_ports and > >observed_ports files. :-( Not that I have any better suggestions... > > Right, I forgot to add a note here, that this follows space > separated list pattern similarly as in case of triggers attribute. > Of course other suggestions are welcome. > > Also a description of the device connected to the port would be a nice > feature, however I am not certain about the feasibility thereof. > >>> > >>>What kind of description do you mean? Where should it be used / where > >>>should it appear? > >>> > >> > >>Product name/symbol. Actually it should be USB subsystem responsibility > >>to provide the means for querying the product name by port id, if it > >>is possible at all. > > > > cat /sys/bus/usb/devices/PORT/product > > cat /sys/bus/usb/devices/PORT/manufacturer > > > >These will work if there is a device registered under PORT. > > I've found only idProduct and idVendor files. They indeed uniquely > identify the device, but the numbers are not human readable. Actually, they don't. They identify device _type_. If you have two mice of the same type connected, they'll have same idProduct / idVendor values. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] Documentation: kdump: remind user of nr_cpus
On 08/25/16 at 01:10pm, Jonathan Corbet wrote: > On Wed, 24 Aug 2016 13:06:45 +0800 > Baoquan He wrote: > > > Hi Jon, do you think change as below is OK to you? > > So nr_cpus is the maximum value, and maxcpus is the current number. > Figures. No wonder the documentation is confusing... Yes. Thanks for reviewing this patchset. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] Documentation: kdump: remind user of nr_cpus
On 08/26/16 at 08:45am, "Zhou, Wenjian/周文剑" wrote: > Hi Baoquan, > > Sorry, I misunderstood it before. > Thanks for your detailed explanation. > > Hi Jon and Baoquan, I'm confused about how to adjust the kdump.txt. > Does the patch set v9 still OK? Yeah, I think it's OK. Let's wait for Jon's feekback. > > -- > Thanks > Zhou > > On 08/24/2016 01:06 PM, Baoquan He wrote: > >On 08/22/16 at 09:14am, "Zhou, Wenjian/周文剑" wrote: > >>On 08/19/2016 11:57 PM, Jonathan Corbet wrote: > >>>On Fri, 19 Aug 2016 08:33:21 +0800 > >>>"Zhou, Wenjian/周文剑" wrote: > >>> > I was also confused by maxcpus and nr_cpus before writing this patch. > I think it is a good choice to describe it in kernel-parameters.txt. > > Then, only two things need to be done I think. > One is move the above description to maxcpus= in kernel-parameters.txt. > And the other is replace maxcpus with maxcpus/nr_cpus in kdump.txt. > > How do you think? > >>> > >>>That is not quite what I had in mind, sorry. What I would really like to > >>>see in kernel-parameters.txt is an explanation of how those two parameters > >>>differ - what do they do differently and how should a user choose one over > >>>the other? What we have now offers no guidance in that matter. > >>> > >> > >>I thought about it. I think user may not need this. > >>What user really want to know is how to choose. > >>And it is also not a hard work. If nr_cpus is not supported by the ARCH, > >>use maxcpus. > >>Otherwise, nr_cpus. The reason why maxcpus still exists is nr_cpus can't be > >>supported > >>by some ARCHes. > > > >I think Jon is suggesting that a note can be added into > >kernel-parameter.txt to tell what's the difference between nr_cpus and > >max_cpus. I checked code and discussed within our kdump team, max_cpus > >is used to limit how many 'present' cpus are allowed to be brought up > >during system bootup, while nr_cpus is used to set the upper limit of > >'possible' cpus. E.g on my laptop, there are 4 cpus while 4 hotplug > >cpus, altogether 8 possible cpus. Possible cpus slot is for cpu hot > >plug, means during bootup you want to bring up 4 present cpus, but > >later you could physically hot plug 4 others. Because of attribute of > >some static percpu variables, we need pre-allocate memory for all > >possible cpus though some of them may not be really used if no extra > >cpu physically hot plugged after system bootup. > > > >Hence for kdump kernel, people never want to do a cpu hot plug in there. > >That's why we want to use nr_cpus to limit the number of possible cpu to > >save memory. E.g still on my laptop, if I want to do a kdump, the number > >of possible cpu is still 8, but you may want to use only 1 cpu to dump, > >maybe 2 or 3 for parallel dumping. But you absolutely don't want to set > >nr_cpus=8 in your kdump kernel cmdline, though it doesn't cause failure, > >memory is wasted because of percpu pre-allocation. So specifying nr_cpus=1 > >is much better. While with specifying max_cpus=1, the number of possible > >cpu is still 8. That's the reason. On x86_64 and s390, there's another > >kernel para "possible_cpus=xx" which can be used to set possible cpus for > >cpu hot plug. Only when "possible_cpus=0" is specified, smp is disabled. > >I am not very sure why this is introduced, number of possible cpu is > >decided by the min value of nr_cpus= and possible_cpus=. > > > >nr_cpus and maxcpus might not be very clear to people which are > >described in Documentation/kernel-parameters.txt. > > > >Hi Jon, do you think change as below is OK to you? > > > > > > From 8b940193a29acf0857d4975d77f4b9f48e2d6cb8 Mon Sep 17 00:00:00 2001 > >From: Baoquan He > >Date: Wed, 24 Aug 2016 11:14:34 +0800 > >Subject: [PATCH] docs: kernel-parameter : Improve the description of nr_cpus > > and maxcpus > > > > From the old description people still can't get what's the exact > >difference between nr_cpus and maxcpus. Especially in kdump kernel > >nr_cpus is always suggested if it's implemented in the ARCH. The > >reason is nr_cpus is used to limit the max number of possible cpu > >in system, the sum of already plugged cpus and hot plug cpus can't > >exceed its value. However maxcpus is used to limit how many cpus > >are allowed to be brought up during bootup. > > > >Signed-off-by: Baoquan He > >--- > > Documentation/kernel-parameters.txt | 20 +--- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > >diff --git a/Documentation/kernel-parameters.txt > >b/Documentation/kernel-parameters.txt > >index 46c030a..25d3b36 100644 > >--- a/Documentation/kernel-parameters.txt > >+++ b/Documentation/kernel-parameters.txt > >@@ -2161,10 +2161,13 @@ bytes respectively. Such letter suffixes can also be > >entirely omitted. > > than or equal to this physical address is ignored. > > > > maxcpus=[SMP] Maximum number of processors that an SMP kernel > >-should make use of. maxcpus=n : n >= 0
[PATCH 1/3] documentation/scsi: Remove nodisconnect parameter
The driver that used the 'nodisconnect' parameter was removed in
commit 565bae6a4a8f ("[SCSI] 53c7xx: kill driver"). Related documentation
was cleaned up in commit f37a7238d379 ("[SCSI] 53c7xx: fix removal
fallout"), except for the remaining two mentions that are removed here.
Signed-off-by: Finn Thain
---
Documentation/kernel-parameters.txt|2 --
Documentation/scsi/scsi-parameters.txt |2 --
2 files changed, 4 deletions(-)
Index: linux/Documentation/kernel-parameters.txt
===
--- linux.orig/Documentation/kernel-parameters.txt 2016-08-27
12:29:57.0 +1000
+++ linux/Documentation/kernel-parameters.txt 2016-08-27 12:29:57.0
+1000
@@ -2543,8 +2543,6 @@ bytes respectively. Such letter suffixes
nodelayacct [KNL] Disable per-task delay accounting
- nodisconnect[HW,SCSI,M68K] Disables SCSI disconnects.
-
nodsp [SH] Disable hardware DSP at boot time.
noefi Disable EFI runtime services support.
Index: linux/Documentation/scsi/scsi-parameters.txt
===
--- linux.orig/Documentation/scsi/scsi-parameters.txt 2016-08-27
12:29:57.0 +1000
+++ linux/Documentation/scsi/scsi-parameters.txt2016-08-27
12:29:57.0 +1000
@@ -79,8 +79,6 @@ parameters may be changed at runtime by
ncr53c8xx= [HW,SCSI]
- nodisconnect[HW,SCSI,M68K] Disables SCSI disconnects.
-
osst= [HW,SCSI] SCSI Tape Driver
Format: ,
See also Documentation/scsi/st.txt.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
