RE: [PATCH net-next] usbnet: fix cyclical race on disconnect with work queue

[Sai Krishna Gajula]

> -----Original Message-----
> From: Oliver Neukum <oneukum@xxxxxxxx>
> Sent: Wednesday, March 27, 2024 8:41 PM
> To: Sai Krishna Gajula <saikrishnag@xxxxxxxxxxx>; Oliver Neukum
> <oneukum@xxxxxxxx>; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: syzbot+9665bf55b1c828bbcd8a@xxxxxxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next] usbnet: fix cyclical race on
> disconnect with work queue
> 
> 
> On 3/22/24 18:43, Sai Krishna Gajula wrote:
> >
> >> -----Original Message-----
> >> From: Oliver Neukum <oneukum@xxxxxxxx>
> >> Sent: Thursday, March 21, 2024 6:17 PM
> >> To: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> >> pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> >> linux-kernel@xxxxxxxxxxxxxxx
> >> Cc: Oliver Neukum <oneukum@xxxxxxxx>;
> >> syzbot+9665bf55b1c828bbcd8a@xxxxxxxxxxxxxxxxxxxxxxxxx
> >> Subject: [PATCH net-next] usbnet: fix cyclical race on disconnect
> >> with work queue
> >
> > This patch seems to be a fix, in that case the subject need to be with
> > [PATCH net]
> 
> OK
> >
> >>
> >> The work can submit URBs and the URBs can schedule the work.
> >> This cycle needs to be broken, when a device is to be stopped.
> >> Use a flag to do so.
> >>
> >> Fixes: f29fc259976e9 ("[PATCH] USB: usbnet (1/9) clean up framing")
> >
> > Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie:
> 'Fixes: f29fc259976e ("[PATCH] USB: usbnet (1/9) clean up framing")'
> 
> Ehm, what exactly did I do differently

Checkpatch triggers warning if we give Fixes tag(f29fc259976e -9-) more than 12 chars. It is advisable to follow the upstreaming process steps to have uniformity across patches.

> 
> >> --- a/drivers/net/usb/usbnet.c
> >> +++ b/drivers/net/usb/usbnet.c
> >> @@ -467,10 +467,12 @@ static enum skb_state defer_bh(struct usbnet
> >> *dev, struct sk_buff *skb,  void usbnet_defer_kevent (struct usbnet
> >> *dev, int work)
> >
> > space prohibited between function name and open parenthesis '('
> 
> I am sorry, but this is the context of the diff. You are not suggesting to mix
> gratitious format changes into a bug fix, are you?

Checkpatch does a primary check and triggered warning if we use space between fn name and '('.  It is advisable to follow the upstreaming process steps(clean checkpatch output) to have uniformity across patches. Also check comments from Gregkh bot regarding this patch.

> 
> >> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> >> index
> >> 9f08a584d707..d26599faab33 100644
> >> --- a/include/linux/usb/usbnet.h
> >> +++ b/include/linux/usb/usbnet.h
> >> @@ -76,8 +76,26 @@ struct usbnet {
> >>   #		define EVENT_LINK_CHANGE	11
> >>   #		define EVENT_SET_RX_MODE	12
> >>   #		define EVENT_NO_IP_ALIGN	13
> >> +/*
> >> + * this one is special, as it indicates that the device is going
> >> +away
> >> + * there are cyclic dependencies between tasklet, timer and bh
> >> + * that must be broken
> >> + */
> >
> > Networking block comments don't use an empty /* line, use /* Comment...
> 
> OK
> 
> 	Regards
> 		Oliver