On Wed, Sep 28, 2016 at 05:25:42PM +0200, Dave Penkler wrote: > On Wed, Sep 28, 2016 at 12:29:35PM +0200, Ladislav Michl wrote: > > On Wed, Sep 28, 2016 at 11:45:02AM +0200, Greg Kroah-Hartman wrote: > > > If it is freed, why is a read even able to happen? Ah, ick, no proper > > > reference counting is happening here :( > > > > > > Oh, no, wait, it is happening properly, it's just that it's not the > > > lifespan that the devm_kzalloc() is attached to, so yes, the correct fix > > > here is to revert that patch as it is incorrect. > > > > Well, reference counting is also suspicious as kref_get(&data->kref) in > > probe function with comment "will reference data in int urb" gives no > > clue why there's explicit reference. Also what if we add classic > > error unwinding and leave usb_submit_urb to open time? But wait, this > > driver allows multiple opens? Is it intentional? It could be done this > > way (note patch is only for reference as there's nothing to prevent > > multiple open and therefore multiple usb_submit_urb): > > > > The usbtmc_interrupt routine will reference data in the int urb from > the interrupt context. Also submitting the urb on probe makes for > simpler housekeeping. Only one interrupt urb needs to be submitted no > matter how many concurrent opens there are for the minor. I will > submit a patch to revert the "convert to devm_kzalloc" patch and make > the comment more explicit. I've already merged such a commit, but if you want to document it better, that would be wonderful. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
