Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> writes: > On 2018-06-12 12:43:01 [-0400], Alan Stern wrote: >> On Tue, 12 Jun 2018, Sebastian Andrzej Siewior wrote: >> >> > In the code path >> > __usb_hcd_giveback_urb() >> > -> wdm_in_callback() >> > -> service_outstanding_interrupt() >> > >> > The function service_outstanding_interrupt() will unconditionally enable >> > interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL. >> > If the HCD completes in BH (like ehci does) then the context remains >> > atomic due local_bh_disable() and enabling interrupts does not change >> > this. >> > >> > Add an argument to service_outstanding_interrupt() which decides >> > whether or not it is save to enable interrupts during unlocking and use >> > GFP_KERNEL or not. >> >> Wouldn't it be easier just to use spin_lock_irqsave() and GFP_ATOMIC >> all the time? That's what people normally do with code that can be >> called in both process and interrupt contexts. > > service_outstanding_interrupt() does unlock + lock instead lock + > unlock. If you want to have this "always" working (without the > argument), we could do the false case: > + spin_unlock(&desc->iuspin); > + rv = usb_submit_urb(desc->response, GFP_ATOMIC); > + spin_lock(&desc->iuspin); > > all the time. I wanted to preserve the GFP_KERNEL part which is probably > used more often but fell that this is not needed I could drop that part. Yes, the atomic case should be rare. It will only happen on errors, and IIUC that's only to work around issues caused by reporting errors back to userspace without actually wanting to err out anyway. I believe it would be better to decide one an error policy and stick to that. Then you could just simplify away that whole mess, by either ignoring the error and continue or bailing out and die. Bjørn -- 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
