| Message ID | 20190318081834.4601-1-guido@kiener-muenchen.de (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 9d6a54c1430647355a5e23434881b2ca3d192b48 |
| Headers | show |
| Series | [v2,1/3] usb: gadget: net2280: Fix overrun of OUT messages | expand |
On Mon, 18 Mar 2019, Guido Kiener wrote: > The OUT endpoint normally blocks (NAK) subsequent packets when a > short packet was received and returns an incomplete queue entry to > the gadget driver. Thereby the gadget driver can detect a short packet > when reading queue entries with a length that is not equal to a > multiple of packet size. > > The start_queue() function enables receiving OUT packets regardless > of the content of the OUT FIFO. This results in a problem: > When receiving is enabled again, more OUT packets are appended to > the last short packet and the gadget driver cannot determine the end > of a short packet anymore. Furthermore the remaining data in the OUT > FIFO is not delivered to the gadget driver immediately and can produce > timeout errors. > > This fix stops OUT naking only when OUT naking is enabled and when the > OUT FIFO is empty. It ensures that all received data is delivered to > the gadget driver which can detect a short packet now before new > packets overrun the last short packet. This description should explain the race which causes the problem. With the current code, it's possible that the "!ep->is_in && (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test will fail, then a short packet will be received, and then start_queue() will call stop_out_naking(). That's what we don't want -- OUT naking gets turned off while there is data in the FIFO. With the patch, this race can't occur because the FIFO's state is tested after we know that OUT naking is already turned on. That patch itself looks good, I just think the explanation should be improved. Alan Stern > Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> > --- > drivers/usb/gadget/udc/net2280.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c > index f63f82450bf4..e0b413e9e532 100644 > --- a/drivers/usb/gadget/udc/net2280.c > +++ b/drivers/usb/gadget/udc/net2280.c > @@ -866,9 +866,6 @@ static void start_queue(struct net2280_ep *ep, u32 dmactl, u32 td_dma) > (void) readl(&ep->dev->pci->pcimstctl); > > writel(BIT(DMA_START), &dma->dmastat); > - > - if (!ep->is_in) > - stop_out_naking(ep); > } > > static void start_dma(struct net2280_ep *ep, struct net2280_request *req) > @@ -907,6 +904,7 @@ static void start_dma(struct net2280_ep *ep, struct net2280_request *req) > writel(BIT(DMA_START), &dma->dmastat); > return; > } > + stop_out_naking(ep); > } > > tmp = dmactl_default; >
Zitat von Alan Stern <stern@rowland.harvard.edu>: > On Mon, 18 Mar 2019, Guido Kiener wrote: > >> The OUT endpoint normally blocks (NAK) subsequent packets when a >> short packet was received and returns an incomplete queue entry to >> the gadget driver. Thereby the gadget driver can detect a short packet >> when reading queue entries with a length that is not equal to a >> multiple of packet size. >> >> The start_queue() function enables receiving OUT packets regardless >> of the content of the OUT FIFO. This results in a problem: >> When receiving is enabled again, more OUT packets are appended to >> the last short packet and the gadget driver cannot determine the end >> of a short packet anymore. Furthermore the remaining data in the OUT >> FIFO is not delivered to the gadget driver immediately and can produce >> timeout errors. >> >> This fix stops OUT naking only when OUT naking is enabled and when the >> OUT FIFO is empty. It ensures that all received data is delivered to >> the gadget driver which can detect a short packet now before new >> packets overrun the last short packet. > > This description should explain the race which causes the problem. > With the current code, it's possible that the "!ep->is_in && > (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test will fail, > then a short packet will be received, and then start_queue() will call > stop_out_naking(). That's what we don't want -- OUT naking gets turned > off while there is data in the FIFO. With the patch, this race can't > occur because the FIFO's state is tested after we know that OUT naking > is already turned on. > Thanks. Please feel free to change my wording/spelling. I'm not offended. Does this description meet your request: The OUT endpoint normally blocks (NAK) subsequent packets when a short packet was received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a race: With the current code, it's possible that the "!ep->is_in && (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test will fail, then a short packet will be received, and then start_queue() will call stop_out_naking(). That's what we don't want -- OUT naking gets turned off while there is data in the FIFO. With the patch, this race can't occur because the FIFO's state is tested after we know that OUT naking is already turned on. Guido > That patch itself looks good, I just think the explanation should be > improved. > > Alan Stern > >> Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> >> --- >> drivers/usb/gadget/udc/net2280.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/usb/gadget/udc/net2280.c >> b/drivers/usb/gadget/udc/net2280.c >> index f63f82450bf4..e0b413e9e532 100644 >> --- a/drivers/usb/gadget/udc/net2280.c >> +++ b/drivers/usb/gadget/udc/net2280.c >> @@ -866,9 +866,6 @@ static void start_queue(struct net2280_ep *ep, >> u32 dmactl, u32 td_dma) >> (void) readl(&ep->dev->pci->pcimstctl); >> >> writel(BIT(DMA_START), &dma->dmastat); >> - >> - if (!ep->is_in) >> - stop_out_naking(ep); >> } >> >> static void start_dma(struct net2280_ep *ep, struct net2280_request *req) >> @@ -907,6 +904,7 @@ static void start_dma(struct net2280_ep *ep, >> struct net2280_request *req) >> writel(BIT(DMA_START), &dma->dmastat); >> return; >> } >> + stop_out_naking(ep); >> } >> >> tmp = dmactl_default; >>
On Mon, 18 Mar 2019 guido@kiener-muenchen.de wrote: > Zitat von Alan Stern <stern@rowland.harvard.edu>: > > > On Mon, 18 Mar 2019, Guido Kiener wrote: > > > >> The OUT endpoint normally blocks (NAK) subsequent packets when a > >> short packet was received and returns an incomplete queue entry to > >> the gadget driver. Thereby the gadget driver can detect a short packet > >> when reading queue entries with a length that is not equal to a > >> multiple of packet size. > >> > >> The start_queue() function enables receiving OUT packets regardless > >> of the content of the OUT FIFO. This results in a problem: > >> When receiving is enabled again, more OUT packets are appended to > >> the last short packet and the gadget driver cannot determine the end > >> of a short packet anymore. Furthermore the remaining data in the OUT > >> FIFO is not delivered to the gadget driver immediately and can produce > >> timeout errors. > >> > >> This fix stops OUT naking only when OUT naking is enabled and when the > >> OUT FIFO is empty. It ensures that all received data is delivered to > >> the gadget driver which can detect a short packet now before new > >> packets overrun the last short packet. > > > > This description should explain the race which causes the problem. > > With the current code, it's possible that the "!ep->is_in && > > (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test will fail, > > then a short packet will be received, and then start_queue() will call > > stop_out_naking(). That's what we don't want -- OUT naking gets turned > > off while there is data in the FIFO. With the patch, this race can't > > occur because the FIFO's state is tested after we know that OUT naking > > is already turned on. > > > > Thanks. Please feel free to change my wording/spelling. I'm not offended. > Does this description meet your request: > > The OUT endpoint normally blocks (NAK) subsequent packets when a > short packet was received and returns an incomplete queue entry to > the gadget driver. Thereby the gadget driver can detect a short packet > when reading queue entries with a length that is not equal to a > multiple of packet size. > > The start_queue() function enables receiving OUT packets regardless > of the content of the OUT FIFO. This results in a race: > With the current code, it's possible that the "!ep->is_in && > (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test will fail, > then a short packet will be received, and then start_queue() will call > stop_out_naking(). That's what we don't want -- OUT naking gets turned > off while there is data in the FIFO. With the patch, this race can't > occur because the FIFO's state is tested after we know that OUT naking > is already turned on. Here are a few more slight changes to the text. Submit the patch again with this description and it will be okay: The OUT endpoint normally blocks (NAK) subsequent packets when a short packet was received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a race: With the current code, it's possible that the "!ep->is_in && (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test in start_dma() will fail, then a short packet will be received, and then start_queue() will call stop_out_naking(). That's what we don't want (OUT naking gets turned off while there is data in the FIFO) because then the next driver request might receive a mixture of old and new packets. With the patch, this race can't occur because the FIFO's state is tested after we know that OUT naking is already turned on, and OUT naking is stopped only when both of the conditions are met. This ensures that all received data is delivered to the gadget driver, which can detect a short packet now before new packets are appended to the last short packet. Alan Stern
Zitat von Alan Stern <stern@rowland.harvard.edu>: > > Here are a few more slight changes to the text. Submit the patch again > with this description and it will be okay: > > The OUT endpoint normally blocks (NAK) subsequent packets when a > short packet was received and returns an incomplete queue entry to > the gadget driver. Thereby the gadget driver can detect a short packet > when reading queue entries with a length that is not equal to a > multiple of packet size. > > The start_queue() function enables receiving OUT packets regardless of > the content of the OUT FIFO. This results in a race: With the current > code, it's possible that the "!ep->is_in && (readl(&ep->regs->ep_stat) > & BIT(NAK_OUT_PACKETS))" test in start_dma() will fail, then a short > packet will be received, and then start_queue() will call > stop_out_naking(). That's what we don't want (OUT naking gets turned > off while there is data in the FIFO) because then the next driver > request might receive a mixture of old and new packets. > > With the patch, this race can't occur because the FIFO's state is > tested after we know that OUT naking is already turned on, and OUT > naking is stopped only when both of the conditions are met. This > ensures that all received data is delivered to the gadget driver, > which can detect a short packet now before new packets are appended > to the last short packet. > Thank you. Sounds good. Guido
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index f63f82450bf4..e0b413e9e532 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -866,9 +866,6 @@ static void start_queue(struct net2280_ep *ep, u32 dmactl, u32 td_dma) (void) readl(&ep->dev->pci->pcimstctl); writel(BIT(DMA_START), &dma->dmastat); - - if (!ep->is_in) - stop_out_naking(ep); } static void start_dma(struct net2280_ep *ep, struct net2280_request *req) @@ -907,6 +904,7 @@ static void start_dma(struct net2280_ep *ep, struct net2280_request *req) writel(BIT(DMA_START), &dma->dmastat); return; } + stop_out_naking(ep); } tmp = dmactl_default;
The OUT endpoint normally blocks (NAK) subsequent packets when a short packet was received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a problem: When receiving is enabled again, more OUT packets are appended to the last short packet and the gadget driver cannot determine the end of a short packet anymore. Furthermore the remaining data in the OUT FIFO is not delivered to the gadget driver immediately and can produce timeout errors. This fix stops OUT naking only when OUT naking is enabled and when the OUT FIFO is empty. It ensures that all received data is delivered to the gadget driver which can detect a short packet now before new packets overrun the last short packet. Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> --- drivers/usb/gadget/udc/net2280.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)