Re: [00/14] can: cleanup of af_can/raw + simplifying of ndev->ml_priv handling

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1.1: Type: text/plain, Size: 6489 bytes --]

On 08/03/2017 07:03 AM, Oliver Hartkopp wrote:
> Hello Marc,
> 
> On 08/02/2017 07:44 PM, Marc Kleine-Budde wrote:
>> Hello,
>>
>> while reviwing and cleaning up the j1939 stack I digged a bit deeper into
>> af_can and raw implementation.
>>
>> The first patch adds a missing error check and will probably go into -stable.
> 
> ack.
> I'm currently working on an additional missing check for register of 
> netdev notifier ...
> 
>> Patches 2-8 change some struct and variable names, making the code more
>> readble, IMHO.
> 
> I'm not convinced on this part.
> Where have you seen, that naming variables identical to the structs is 
> common or better readable?

More readable than 1 and 2 character variable names at least :)

>> Patch 9 removed the need for struct raw_sock::ifindex from the raw sock, by
>> using struct sock::sk_bound_dev_if from the generic socket structure.
> 
> Have a bad feeling on that - will comment later.

Ok

>> Patch 10: Checks if can_family is AF_CAN in raw's bind function.
> fine.
> 
>> Patch 11: Cleans up the newly integrated CAN net namespace support.
> 
> need to review
> 
>> Patches 13-14: Where to put the per device protocol specific memory? af_can
>> allocated it's memory during a netdev_notifier call, life cycle proves to be
>> rather complicated (see remove_on_zero_entries, etc...), adding the j1939
>> memory makes it even more compilcated. So I decided to allocate the memory
>> during the allocation if net_device. And this seems to work. More details in
>> the individual patches.
> 
> 'Seems to work' sounds frightening. There was a racy reason to have that 
> implementation as-is. Although this approach sounds interesting. Need to 
> review too.

This is the callstack, when unplugging a USB device:

> [   53.144726] [<c0016cfc>] (unwind_backtrace) from [<c0013d04>] (show_stack+0x18/0x1c)
> [   53.152692] [<c0013d04>] (show_stack) from [<bf016510>] (can_rx_unregister+0x68/0x1c0 [can])
> [   53.161366] [<bf016510>] (can_rx_unregister [can]) from [<bf01f0c0>] (raw_disable_filters+0x44/0x60 [can_raw])
> [   53.171586] [<bf01f0c0>] (raw_disable_filters [can_raw]) from [<bf01f16c>] (raw_notifier+0x90/0x11c [can_raw])
> [   53.181799] [<bf01f16c>] (raw_notifier [can_raw]) from [<c0042000>] (notifier_call_chain+0x64/0xa4)
> [   53.191038] [<c0042000>] (notifier_call_chain) from [<c0042068>] (raw_notifier_call_chain+0x1c/0x24)
> [   53.200389] [<c0042068>] (raw_notifier_call_chain) from [<c03bc3c0>] (call_netdevice_notifiers+0x14/0x1c)
> [   53.210153] [<c03bc3c0>] (call_netdevice_notifiers) from [<c03bf634>] (rollback_registered_many+0x1a8/0x358)
> [   53.220180] [<c03bf634>] (rollback_registered_many) from [<c03bf82c>] (rollback_registered+0x48/0x68)
> [   53.229587] [<c03bf82c>] (rollback_registered) from [<c03bf8e0>] (unregister_netdevice_queue+0x94/0xb0)
> [   53.239280] [<c03bf8e0>] (unregister_netdevice_queue) from [<c03bf91c>] (unregister_netdev+0x20/0x28)
> [   53.248693] [<c03bf91c>] (unregister_netdev) from [<bf011268>] (gs_usb_disconnect+0x4c/0x7c [gs_usb])
> [   53.258112] [<bf011268>] (gs_usb_disconnect [gs_usb]) from [<c03515b0>] (usb_unbind_interface+0x80/0x1b4)
> [   53.267872] [<c03515b0>] (usb_unbind_interface) from [<c0324e00>] (device_release_driver_internal+0x124/0x1b4)
> [   53.278062] [<c0324e00>] (device_release_driver_internal) from [<c0323bb8>] (bus_remove_device+0xf0/0x124)
> [   53.287902] [<c0323bb8>] (bus_remove_device) from [<c03209b0>] (device_del+0x210/0x2c8)
> [   53.296089] [<c03209b0>] (device_del) from [<c035023c>] (usb_disable_device+0xa4/0x224)
> [   53.304275] [<c035023c>] (usb_disable_device) from [<c034826c>] (usb_disconnect+0x90/0x17c)
> [   53.312801] [<c034826c>] (usb_disconnect) from [<c0349000>] (hub_event+0x524/0xf74)
> [   53.320632] [<c0349000>] (hub_event) from [<c0039548>] (process_one_work+0x344/0x684)
> [   53.328653] [<c0039548>] (process_one_work) from [<c003ac7c>] (worker_thread+0x2b4/0x410)
> [   53.337013] [<c003ac7c>] (worker_thread) from [<c00407c8>] (kthread+0x134/0x154)
> [   53.344589] [<c00407c8>] (kthread) from [<c000fa6c>] (ret_from_fork+0x14/0x28)

The raw protocol is teared down within the unregister_netdev() via a
call_netdevice_notifiers()...

> [   53.423328] [<c0016cfc>] (unwind_backtrace) from [<c0013d04>] (show_stack+0x18/0x1c)
> [   53.434197] [<c0013d04>] (show_stack) from [<c03e4a5c>] (netdev_release+0x18/0x3c)
> [   53.445085] [<c03e4a5c>] (netdev_release) from [<c031fac8>] (device_release+0x64/0x9c)
> [   53.456348] [<c031fac8>] (device_release) from [<c048a904>] (kobject_put+0xd8/0x1d8)
> [   53.467330] [<c048a904>] (kobject_put) from [<bf011278>] (gs_usb_disconnect+0x5c/0x7c [gs_usb])
> [   53.479362] [<bf011278>] (gs_usb_disconnect [gs_usb]) from [<c03515b0>] (usb_unbind_interface+0x80/0x1b4)
> [   53.492137] [<c03515b0>] (usb_unbind_interface) from [<c0324e00>] (device_release_driver_internal+0x124/0x1b4)
> [   53.505308] [<c0324e00>] (device_release_driver_internal) from [<c0323bb8>] (bus_remove_device+0xf0/0x124)
> [   53.518221] [<c0323bb8>] (bus_remove_device) from [<c03209b0>] (device_del+0x210/0x2c8)
> [   53.529664] [<c03209b0>] (device_del) from [<c035023c>] (usb_disable_device+0xa4/0x224)
> [   53.540867] [<c035023c>] (usb_disable_device) from [<c034826c>] (usb_disconnect+0x90/0x17c)
> [   53.560981] [<c0349000>] (hub_event) from [<c0039548>] (process_one_work+0x344/0x684)
> [   53.569453] [<c0039548>] (process_one_work) from [<c003ac7c>] (worker_thread+0x2b4/0x410)
> [   53.578303] [<c003ac7c>] (worker_thread) from [<c00407c8>] (kthread+0x134/0x154)
> [   53.586337] [<c00407c8>] (kthread) from [<c000fa6c>] (ret_from_fork+0x14/0x28)

...while the netdev is finally discarded via the kobject_put later on.

> I'm currently pretty busy at work.

That's my world, too :)

> Please to not push these things without my ACK (as we had it with the 
> namespace support where I crashed my easter holiday to fix/implement all 
> the missing stuff to fit the merge window).

I'm on holidays too, SHA2017. If someone wants so meet me in person and
drink a mate or beer contact me.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]