MIME-Version: 1.0
References: <20230307005028.2065800-1-grundler@chromium.org>
 <84094771-7f98-0d8d-fe79-7c22e15a602d@gmail.com> <CANEJEGvM_xLrSSjrgKLh_xP+BrFFT+afDQhG8BOdgHPf7eR4gQ@mail.gmail.com>
 <20230307102931.GA25631@wunner.de>
In-Reply-To: <20230307102931.GA25631@wunner.de>
From:   Grant Grundler <grundler@chromium.org>
Date:   Tue, 7 Mar 2023 10:13:26 -0800
Message-ID: <CANEJEGv44GeY=qWiVTJx6tkL-fxs=7vT8uzGm9nPsGirwVBGVQ@mail.gmail.com>
Subject: Re: [PATCH] net: asix: fix modprobe "sysfs: cannot create duplicate filename"
To:     Lukas Wunner <lukas@wunner.de>
Cc:     Grant Grundler <grundler@chromium.org>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Oleksij Rempel <linux@rempel-privat.de>,
        Pavel Skripkin <paskripkin@gmail.com>,
        Anton Lundin <glance@acc.umu.se>,
        Eizan Miyamoto <eizan@chromium.org>,
        Jakub Kicinski <kuba@kernel.org>,
        netdev <netdev@vger.kernel.org>,
        "David S . Miller" <davem@davemloft.net>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 7, 2023 at 2:29=E2=80=AFAM Lukas Wunner <lukas@wunner.de> wrote=
:
>
> On Mon, Mar 06, 2023 at 10:10:09PM -0800, Grant Grundler wrote:
> > On Mon, Mar 6, 2023 at 7:46???PM Florian Fainelli <f.fainelli@gmail.com=
> wrote:
> > > On 3/6/2023 4:50 PM, Grant Grundler wrote:
> > > > +     priv->phydev =3D mdiobus_get_phy(priv->mdio, priv->phy_addr);
> > > > +     if (priv->phydev)
> > > > +             return 0;
> > >
> > > This was in ax88772_init_phy() before, why is this being moved here n=
ow?
> >
> > Because other drivers I looked at (e.g. tg3 and r8169) do all the mdiob=
us_*
> > calls in one function and I wanted to have some "symmetry"
> > with ax88772_release_mdio() function I added below.
>
> I'd suggest moving this cleanup to a separate commit so that you keep
> the fix itself as small as possible and thus minimize the potential of
> introducing regressions in stable kernels that will receive the fix.

Ok - will do.

> Also, per convention please use the if-clause to catch the error case,
> not the success case.  It doesn't matter if you need two or three more
> lines, readability is more important IMO.

Sure. No problem. The code I wrote seemed easier to read but I'm
familiar with the convention and have no objection to that.

I'll add the Fixes: tag as well (thanks for confirming Oleksij!)

cheers,
grant

>
> Thanks,
>
> Lukas