| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 22 Oct 2021 08:23:49 +0200
From: Philipp Hortmann <philipp.g.hortmann@...il.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: corbet@....net, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2] Docs: usb: update struct usb_driver, __init and __exit
On 10/21/21 10:00 AM, Greg KH wrote:
> On Wed, Oct 20, 2021 at 10:14:46PM +0200, Philipp Hortmann wrote:
>> update struct usb_driver from usb-skeleton.c.
>> update __init and __exit functions that are moved from
>> usb-skeleton.c to common used multi-stage macros.
>>
>> Signed-off-by: Philipp Hortmann <philipp.g.hortmann@...il.com>
>> ---
>> V1 -> V2: changed :c:func:`usb_register` to usb_register()
>> changed the :c:func:`usb_deregister` to usb_deregister()
>> used literal blocks for makro module_usb_driver and added one more
>> stage of multi-stage macros.
>>
>> .../driver-api/usb/writing_usb_driver.rst | 70 ++++++++++---------
>> 1 file changed, 36 insertions(+), 34 deletions(-)
>>
>> diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
>> index 2176297e5765..12e0481cceae 100644
>> --- a/Documentation/driver-api/usb/writing_usb_driver.rst
>> +++ b/Documentation/driver-api/usb/writing_usb_driver.rst
>> @@ -54,12 +54,15 @@ information is passed to the USB subsystem in the :c:type:`usb_driver`
>> structure. The skeleton driver declares a :c:type:`usb_driver` as::
>>
>> static struct usb_driver skel_driver = {
>> - .name = "skeleton",
>> - .probe = skel_probe,
>> - .disconnect = skel_disconnect,
>> - .fops = &skel_fops,
>> - .minor = USB_SKEL_MINOR_BASE,
>> - .id_table = skel_table,
>> + .name = "skeleton",
>> + .probe = skel_probe,
>> + .disconnect = skel_disconnect,
>> + .suspend = skel_suspend,
>> + .resume = skel_resume,
>> + .pre_reset = skel_pre_reset,
>> + .post_reset = skel_post_reset,
>> + .id_table = skel_table,
>> + .supports_autosuspend = 1,
>
> Why remove the tabs? Is that needed here?
You are right will be changed.
>
>> };
>>
>>
>> @@ -81,36 +84,35 @@ this user-space interaction. The skeleton driver needs this kind of
>> interface, so it provides a minor starting number and a pointer to its
>> :c:type:`file_operations` functions.
>>
>> -The USB driver is then registered with a call to :c:func:`usb_register`,
>> -usually in the driver's init function, as shown here::
>> -
>> - static int __init usb_skel_init(void)
>> - {
>> - int result;
>> -
>> - /* register this driver with the USB subsystem */
>> - result = usb_register(&skel_driver);
>> - if (result < 0) {
>> - err("usb_register failed for the "__FILE__ "driver."
>> - "Error number %d", result);
>> - return -1;
>> - }
>> -
>> - return 0;
>> - }
>> - module_init(usb_skel_init);
>> -
>> +The USB driver is then registered with a call to usb_register()
>> +which is usually in the driver's init function. Since this functionality
>> +is usable with many USB drivers, it is hidden behind multi-stage macros.
>
> I don't understand the need for the "multi-stage macros" term here.
I am not a native English speaker so "multi-stage macros" is just not a
fitting wording. May be “staged macros” is better or something else…
>
> And what functionality is referred to here by "this"?
The “this” is replacing the “init function” but when this is unclear I
will change in a later proposal…
>
>
>> +While the first macros are USB specific the later macros are used in different
>> +subsystems. This removes a lot of boilerplate code.
>
> What later macros? Is that really needed to describe here?
I will improve wording...
> I think the above code example should remain, as it is good for learning
> and understanding, and maybe just add something that says "Or you can
> use the following macro to replace all of the above common code."
I understand the need for keeping the code examples. But I would like to
inform the reader about the macros first.
>
>
>>
>> When the driver is unloaded from the system, it needs to deregister
>> -itself with the USB subsystem. This is done with the :c:func:`usb_deregister`
>> -function::
>> -
>> - static void __exit usb_skel_exit(void)
>> - {
>> - /* deregister this driver with the USB subsystem */
>> - usb_deregister(&skel_driver);
>> - }
>> - module_exit(usb_skel_exit);
>> +itself with the USB subsystem. This is done with usb_deregister()
>> +which is also hidden behind multi-stage macros.
>> +
>> +The init and exit functions are included in the macro module_usb_driver.
>> +Find the first three stages of macros below::
>> +
>> + module_usb_driver(skel_driver);
>> + |
>> + V
>> + module_driver(__usb_driver, usb_register, usb_deregister)
>> + | \ \
>> + V ---------- ----------
>> + static int __init __driver##_init(void) \ | |
>> + { \ v--------------------------- |
>> + return __register(&(__driver) , ##__VA_ARGS__); \ |
>> + } \ |
>> + module_init(__driver##_init); \ |
>> + static void __exit __driver##_exit(void) \ |
>> + { \ v------------------------------------------------
>> + __unregister(&(__driver) , ##__VA_ARGS__); \
>> + } \
>> + module_exit(__driver##_exit);
>
> As the one who wrote these macros, I can't really understand the
> ascii-art here, so I worry about anyone else :)
Code is just better readable, even when code uses more lines. Will be
changed in next proposal.
>
> Again, do not think trying to show an implementation detail like this is
> needed.
The big question for me is for whom is this document written? For the
USB subsystem maintainer that has even written the code by himself? I
guess not, but may be I am wrong. Or for the kernel newbies like me?
Please consider that the changed lines are may be not so much of use for
me anymore as I am in the details.
When I saw the __init and __exit code example first, I was very happy to
see it and then I was searching in the code for it. I did not find
“init” and “exit” and was very frustrated. I want to help others to get
into this example more smoothly.
>
> thanks,
>
> greg k-h
>
Thanks that you replied at all.
Thanks for your very fast reply.
Kernelnewbie Philipp G. Hortmann
Powered by blists - more mailing lists