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

[Justin Lai <justinlai0215@realtek.com>]

> …
> > when requesting irq, because the first group of interrupts needs to
> > process more events, the overall structure will be different from
> > other groups of interrupts, so it needs to be processed separately.
> 
> Can such a change description become clearer anyhow?

Do you think it's ok if I change the description to the following?
"When requesting interrupt, because the first group of interrupts needs
to process more events, the overall structure and interrupt handler will
be different from other groups of interrupts, so it needs to be handled
separately. The first set of interrupt handlers need to handle the
interrupt status of RXQ0 and TXQ0, TXQ4~7, while other groups of
interrupt handlers will handle the interrupt status of RXQ1&TXQ1 or
RXQ2&TXQ2 or RXQ3&TXQ3 according to the interrupt vector."

> 
> 
> …
> > +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> …
> > +static int rtase_alloc_desc(struct rtase_private *tp) {
> …
> > +                     netdev_err(tp->dev, "Failed to allocate dma
> memory of "
> > +                                         "tx descriptor.\n");
> …
> 
> Would you like to keep the message (from such string literals) in a single line?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
> ntation/process/coding-style.rst?h=v6.10-rc3#n116
> 

Yes, I will make corrections to keep the message in a single line.

> 
> …
> > +static int rtase_alloc_rx_skb(const struct rtase_ring *ring,
> …
> > +{
> …
> > +     struct sk_buff *skb = NULL;
> …
> > +     int ret = 0;
> …
> > +     if (!page) {
> > +             netdev_err(tp->dev, "failed to alloc page\n");
> > +             goto err_out;
> …
> > +     if (!skb) {
> …
> > +             netdev_err(tp->dev, "failed to build skb\n");
> > +             goto err_out;
> > +     }
> …
> > +     return ret;
> 
> I find the following statement more appropriate.
> 
>         return 0;

Thank you, I will make the changes.

> 
> 
> > +
> > +err_out:
> > +     if (skb)
> > +             dev_kfree_skb(skb);
> 
> Why would you like to repeat such a check after it can be determined from the
> control flow that the used variable contains still a null pointer?

Indeed, it seems unnecessary to make this judgment here, I will remove it.

> 
> 
> > +
> > +     ret = -ENOMEM;
> > +     rtase_make_unusable_by_asic(desc);
> > +
> > +     return ret;
> > +}
> …
> 
> It seems that the following statement can be more appropriate.
> 
>         return -ENOMEM;

Ok, I will modify it.

> 
> 
> May the local variable “ret” be omitted here?

Yes, it seems like "ret" is no longer needed.

> 
> 
> …
> > +static int rtase_open(struct net_device *dev) {
> …
> > +     int ret;
> > +
> > +     ivec = &tp->int_vector[0];
> > +     tp->rx_buf_sz = RTASE_RX_BUF_SIZE;
> > +
> > +     ret = rtase_alloc_desc(tp);
> > +     if (ret)
> > +             goto err_free_all_allocated_mem;
> …
> 
> I suggest to return directly after such a resource allocation failure.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
> ntation/process/coding-style.rst?h=v6.10-rc3#n532
> 
> 
> How do you think about to increase the application of scope-based resource
> management?
> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/cleanup.h#L8

Due to our tx and rx each having multiple queues that need to
allocate descriptors, if any one of the queues fails to allocate,
rtase_alloc_desc() will return an error. Therefore, using 'goto'
here rather than directly returning seems to be reasonable.

> 
> Regards,
> Markus