Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]

> On Wed, Jun 12, 2019 at 11:53:03AM +0200, Lorenzo Bianconi wrote:
> > > On Fri, May 31, 2019 at 11:38:23AM +0200, Lorenzo Bianconi wrote:
> > 
> > [...]
> > 
> > > >  	}
> > > >  
> > > >  	urb->num_sgs = max_t(int, i, urb->num_sgs);
> > > > -	urb->transfer_buffer_length = urb->num_sgs * q->buf_size,
> > > > +	urb->transfer_buffer_length = urb->num_sgs * data_size;
> > > >  	sg_init_marker(urb->sg, urb->num_sgs);
> > > >  
> > > >  	return i ? : -ENOMEM;
> > > > @@ -611,8 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
> > > >  	if (!q->entry)
> > > >  		return -ENOMEM;
> > > >  
> > > > -	q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > > > +	if (dev->usb.sg_en)
> > > > +		q->buf_size = MT_BUF_WITH_OVERHEAD(MT_RX_BUF_SIZE);
> > > 
> > > I strongly recommend to not doing this. While this should work
> > > in theory creating buffer with size of 2k + some bytes might
> > > trigger various bugs in dma mapping or other low level code.
> > 
> > even in practice actually :)
> 
> I wouldn't be sure about this. It's not common to have buffers of
> such size and crossing pages boundaries. It really can trigger
> nasty bugs on various IOMMU drivers.

I was just joking, I mean that it worked in the tests I carried out, but I
agree it can trigger some issues in buggy IOMMU drivers

> 
> > but we can be more cautious since probably copying
> > the first 128B will not make any difference
> 
> Not sure if I understand what you mean.

Please correct me if I am wrong but I think max amsdu rx size is 3839B for
mt76. For the sg_en case this frame will span over multiple sg buffers since
sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into account
skb_shared_info when configuring the sg buffer size we will need to always copy
the first 128B of the first buffer since received len will be set to 2048 and
the following if condition will always fail:

if (SKB_WITH_OVERHEAD(buf_size) >= MT_DMA_HDR_LEN + len) {
}

> 
> > > And skb_shered_info is needed only in first buffer IIUC.
> > > 
> > > Also this patch seems to make first patch unnecessary except for
> > > non sg_en case (in which I think rx AMSDU is broken anyway),
> > > so I would prefer just to apply first patch.
> > 
> > I do not think rx AMSDU is broken for non sg_en case since the max rx value
> > allowed should be 3839 IIRC and we alloc one page in this case
> 
> If that's the case we should be fine, but then I do not understand
> why we allocate 8*2k buffers for sg_en case, isn't that AP can
> sent AMSDU frame 16k big?

Sorry I did not get what you mean here, could you please explain?

Regards,
Lorenzo

> 
> Stanislaw
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]