RE: [PATCH net-next v18 02/13] rtase: Implement the .ndo_open function

[Justin Lai]

> 
> > From: Justin Lai <justinlai0215@xxxxxxxxxxx>
> > Sent: Thursday, May 9, 2024 2:29 PM
> > > > +
> > > > +     /* rx and tx descriptors needs 256 bytes alignment.
> > > > +      * dma_alloc_coherent provides more.
> > > > +      */
> > > > +     for (i = 0; i < tp->func_tx_queue_num; i++) {
> > > > +             tp->tx_ring[i].desc =
> > > > +                             dma_alloc_coherent(&pdev->dev,
> > > > +
> > > RTASE_TX_RING_DESC_SIZE,
> > > > +
> > > &tp->tx_ring[i].phy_addr,
> > > > +
> GFP_KERNEL);
> > > > +             if (!tp->tx_ring[i].desc)
> > > You have handled errors gracefully very where else. why not here ?
> >
> > I would like to ask you, are you referring to other places where there
> > are error description messages, but not here?
> other functions, you are freeing allocated resources in case of failure, but here,
> you are returning error directly.
> 
After returning the error, I will do the corresponding error handling in rtase_open.
.
> 
> > > Did you mark the skb for recycle ? Hmm ... did i miss to find the code ?
> > >
> > We have done this part when using the skb and before finally releasing
> > the skb resource. Do you think it would be better to do this part of
> > the process when allocating the skb?
> i think, you added skb_for_recycle() in the following patch. Sorry I missed it .
> ignore my comment.
> 

OK, thank you for your feedback.

> >
> > > > +
> > > > +err_free_all_allocated_irq:
> > > You are allocating from i = 1, but freeing from j = 0;
> >
> > Hi Ratheesh,
> > I have done request_irq() once before the for loop, so there should be
> > no problem starting free from j=0 here.
> Thanks for pointing out.

Thank you also for your review.