Re: [PATCH net] net: qmi_wwan: fix Oops while disconnecting

[I don't have anything more to add to the generic usbnet discussion, but
 want to comment on the qmi_wwan issues]

Ming Lei [off-list ref] writes:

Also, the clear of intfdata may not be observed in .manage_power
since usb_set_intfdata(, NULL) may be completed after the lock wdm_mutex
operation.

True, but irrelevant.  The pointer is either valid or NULL.  We don't
need to care about synchronizing the exact time it is set to NULL.

The locking in cdc-wdm will ensure that the pointer is valid while it is
in use by .manage_power, because usbnet_disconnect is prevented from
continuing with free_netdev() while any caller of .manage_power is
running.

So it is only the sync mechanism that  works on the race even the check is
added in the patch.  Putting usb_set_intfdata(, NULL) after driver_info->unbind
should be OK, and it is a general solution for the problem.

There is no problem wrt qmi_wwan and intfdata as long as the NULL test
is added to .manage_power.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 25, 2012 at 3:24 PM, Bjørn Mork [off-list ref] wrote:

True, but irrelevant.  The pointer is either valid or NULL.  We don't
need to care about synchronizing the exact time it is set to NULL.

The locking in cdc-wdm will ensure that the pointer is valid while it is
in use by .manage_power, because usbnet_disconnect is prevented from
continuing with free_netdev() while any caller of .manage_power is
running.

What I mean is that the situation is just what moving the set to NULL
is doing.

quoted

So it is only the sync mechanism that  works on the race even the check is
added in the patch.  Putting usb_set_intfdata(, NULL) after driver_info->unbind
should be OK, and it is a general solution for the problem.

There is no problem wrt qmi_wwan and intfdata as long as the NULL test
is added to .manage_power.

It depends on the ARCH or compiler.

Considered there is not any locking/memory barrier between the set to NULL
and read the pointer, also no ACCESS_ONCE on read or store the pointer,
reading in .manage_power may see a invalid pointer if the CPU doesn't
support Store Atomicity or the compiler does a byte-at-a-time optimization
on the store[1].

So why not take the correct way in theory? also it is a general solution,
and we can document its usage.


[1], Paul mentioned it in the previous discussion
http://lkml.org/lkml/2012/6/6/280

Thanks,
-- 
Ming Lei