On Fri. 10 juin 2022 at 09:05, Vincent Mailhol <vincent.mailhol@xxxxxxxxx> wrote: > On Fri. 10 Jun 2022 at 05:53, Rhett Aultman <rhett.aultman@xxxxxxxxxxx> wrote: > > The gs_usb driver allocates DMA memory with usb_alloc_coherent() in > > gs_can_open() and then keeps this memory in an URB, with the expectation > > that the memory will be freed when the URB is killed in gs_can_close(). > > Memory allocated with usb_alloc_coherent() cannot be freed in this way > > and much be freed using usb_free_coherent() instead. This means that > ^^^^ > and must > > > repeated cycles of calling gs_can_open() and gs_can_close() will lead to > > a memory leak. > > > > Historically, drivers have handled this by keeping an array of pointers > > to their DMA rx buffers and explicitly freeing them. For an example of > > this technique used in the esd_usb2 driver, see here: > > https://www.spinics.net/lists/linux-can/msg08203.html > > > > While the above method works, the conditions that cause this leak are > > not apparent to driver writers and the method for solving it at the > > driver level has been piecemeal. This patch makes use of a new > > URB_FREE_COHERENT flag on the URB, reducing the solution of this memory > > leak down to a single line of code. > > One more thing, for bug fixes, the best practice is to add a Fixes tag. c.f.: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices") > > Signed-off-by: Rhett Aultman <rhett.aultman@xxxxxxxxxxx> > > --- > > drivers/net/can/usb/gs_usb.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c > > index b29ba9138866..3ded3e14c830 100644 > > --- a/drivers/net/can/usb/gs_usb.c > > +++ b/drivers/net/can/usb/gs_usb.c > > @@ -768,7 +768,7 @@ static int gs_can_open(struct net_device *netdev) > > buf, > > dev->parent->hf_size_rx, > > gs_usb_receive_bulk_callback, parent); > > > > - urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > > + urb->transfer_flags |= (URB_NO_TRANSFER_DMA_MAP | URB_FREE_COHERENT); > > Nitpick but parenthesis are not needed here. Also, there are no > reasons to do a |=. I would prefer to see this: > urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_FREE_COHERENT; > > > usb_anchor_urb(urb, &parent->rx_submitted); > > I looked at the code of gs_usb, this driver has a lot of dust. What > strikes me the most is that it uses usb_alloc_coherent() each time in > its xmit() function instead of allocating the TX URB once in the > open() function and resubmitting then in the callback function. That > last comment is not a criticism of this patch. But if someone has the > motivation to do some cleaning, go ahead… > > Aside from the nitpicks, the patch looks good to me. You can add this > to your v3: > Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > > Yours sincerely, > Vincent Mailhol
