Re: [PATCH v3 2/3] mt76usb: use synchronous msg for mcu command responses

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Fri, Feb 22, 2019 at 10:53:19AM +0100, Lorenzo Bianconi wrote:
> > Use usb_bulk_msg for reading MCU command responses. This simplify code
> > a lot.
> > 
> > Together with 97a3005759c ("mt76usb: allow mt76u_bulk_msg be used
> > for reads") it also fix possible problems with rx data buffers
> > not being aligned and contained within single page. After doing
> > page_frag_alloc(1024) consecutive page_frag_alloc(PAGE_SIZE) will
> > alloc PAGE_SIZE buffer at PAGE_SIZE - 1024 offset.
> > 
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> >  drivers/net/wireless/mediatek/mt76/mt76.h          |  3 +-
> >  drivers/net/wireless/mediatek/mt76/mt76x0/usb.c    | 11 --------
> >  .../net/wireless/mediatek/mt76/mt76x02_usb_mcu.c   | 32 +++++++---------------
> >  drivers/net/wireless/mediatek/mt76/mt76x2/usb.c    | 11 --------
> >  drivers/net/wireless/mediatek/mt76/usb.c           |  1 -
> >  drivers/net/wireless/mediatek/mt76/usb_mcu.c       | 31 +++------------------
> >  6 files changed, 15 insertions(+), 74 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> > index 6092646014c4..c9b5eb9b0582 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> > @@ -384,8 +384,7 @@ struct mt76_usb {
> >  
> >  	struct mt76u_mcu {
> >  		struct mutex mutex;
> > -		struct completion cmpl;
> 
> Hi Stanislaw,
> 
> I was reviewing this approach and I guess it is a little bit racey since now we
> are not sure that when the device is removed or suspended the pending mcu commands
> are terminated and we do not have any api to stop usb transactions.
> Are we sure when we access mt76x02_dev/mt76_dev structure it has not been
> already removed?
> Maybe we need to maintain the completion in mt76u_mcu and use it to wait the mcu
> commands are terminated.

I don't think so. On suspend we do mt76xxu_mac_stop() which access registers
using mcu commands, all should be synchronized by mcu->mutex . All other
process that could use mcu command should be already stopped. Actually suspend
can be simplified.

Device remove should be properly handled by -ENODEV error returned by
usb_bulk_msg.

Stanislaw