linux-kernel - Re: [PATCH] <driver/net/ethernet/amd/nmclan

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] [day] [month] [year] [list]

Message-ID: <Y5SHPK0t821eX9Bw@debian.me>
Date:   Sat, 10 Dec 2022 20:18:52 +0700
From:   Bagas Sanjaya <bagasdotme@...il.com>
To:     Takahashi Akari <akaritakahashioss@...il.com>,
        linux-kernel@...r.kernel.org, Networking <netdev@...r.kernel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Haoyue Xu <xuhaoyue1@...ilicon.com>,
        Guofeng Yue <yueguofeng@...ilicon.com>,
        Geoff Levand <geoff@...radead.org>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH] <driver/net/ethernet/amd/nmclan_cs.c> Remove unnecessary
 line

On Sat, Dec 10, 2022 at 05:47:13PM +0900, Takahashi Akari wrote:
> Hello:
> 
> I sent a patch. Please review and merge.
> 
> Reason:
> Remove unnecessaty line. (if statement is always true)
> 
> Git repository URL:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> 
> File:
> drivers/net/ethernet/amd/nmclan_cs.c : 931
> 

Hi and welcome to LKML!

Based on your patch, here are my notes:

  * Write the patch description in imperative mood (e.g. it should have
    been "Remove redundant inner tx_irq_disabled conditional in
    mace_interrupt() since it has already been handled". Wrap it in about
    72-75 column wide (to account indentation in git-log(1) so that the
    total line length is at maximum 80).
  * Write also short but concise patch subject (one-line summary), while also
    paying attention to the subject prefix. In this
    case, it should have been "net: amd: remove inner tx_irq_disabled
    conditional".
  * Your patch looks like corrupted (tabs converted to spaces, linewrapped,
    etc.). Please configure your email-client not to do such things, or better
    yet, use git-send-email(1) to send patches.
  * You need to Cc: relevant maintainers and lists, which can be obtained
    by `scripts/get_maintainer.pl -norolestats -separator , -- 
    /path/to/your/patch`. Personally I put lists in To: header and individual
    addresses in Cc:. I have added them for you.
  * Last but not least, build the kernel with your patch applied (preferably
    enable W=1 and CONFIG_WERROR and also cross-compile).

See also tips from one of kernel subsystem maintainer at [1].

> Signed-off-by: Takahashi Akari <akaritakahashioss@...il.com>
> ---
>  drivers/net/ethernet/amd/nmclan_cs.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/nmclan_cs.c b/drivers/net/ethernet/amd/nmclan_cs.c
> index 823a329a921f..a90e0c5b603d 100644
> --- a/drivers/net/ethernet/amd/nmclan_cs.c
> +++ b/drivers/net/ethernet/amd/nmclan_cs.c
> @@ -928,10 +928,7 @@ static irqreturn_t mace_interrupt(int irq, void *dev_id)
>  
>    if (lp->tx_irq_disabled) {
>      const char *msg;
> -    if (lp->tx_irq_disabled)
> -      msg = "Interrupt with tx_irq_disabled";
> -    else
> -      msg = "Re-entering the interrupt handler";
> +    msg = "Interrupt with tx_irq_disabled";
>      netdev_notice(dev, "%s [isr=%02X, imr=%02X]\n",
>  		  msg,
>  		  inb(ioaddr + AM2150_MACE_BASE + MACE_IR),

Dunno if you need to also handle the else case (that gives "Re-entering" msg)
in the outer conditional.

Thanks.

[1]: https://lore.kernel.org/all/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/

-- 
An old man doll... just what I always wanted! - Clara

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)