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

[Lorenzo Bianconi <lorenzo.bianconi@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.

ack, I missed mcu->mutex. Thx.

Regards,
Lorenzo

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