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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 17 Jun 2024 09:25:48 +0000
From: Justin Lai <justinlai0215@...ltek.com>
To: Markus Elfring <Markus.Elfring@....de>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "Eric
 Dumazet" <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>,
        Simon Horman <horms@...nel.org>
CC: LKML <linux-kernel@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Hariprasad Kelam <hkelam@...vell.com>, Jiri Pirko <jiri@...nulli.us>,
        "Larry
 Chiu" <larry.chiu@...ltek.com>,
        Ping-Ke Shih <pkshih@...ltek.com>,
        "Ratheesh
 Kannoth" <rkannoth@...vell.com>
Subject: RE: [PATCH net-next v20 02/13] rtase: Implement the .ndo_open function

> …
> > 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