linux-kernel - Re: [PATCH net-next v1] lan78xx: lan7801 MAC support with lan8841

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]

Message-ID: <d3340e88870bb3a63355b729ffe41b6792a1c2da.camel@microchip.com>
Date: Thu, 20 Jun 2024 05:33:57 +0000
From: <Rengarajan.S@...rochip.com>
To: <kuba@...nel.org>
CC: <linux-usb@...r.kernel.org>, <davem@...emloft.net>,
	<Woojung.Huh@...rochip.com>, <linux-kernel@...r.kernel.org>,
	<pabeni@...hat.com>, <netdev@...r.kernel.org>, <edumazet@...gle.com>,
	<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next v1] lan78xx: lan7801 MAC support with lan8841

Hi Jakub,

Apologies for the delay in reply. Thanks for reviewing the patch and
please find my comments inline.

On Wed, 2024-06-12 at 18:33 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, 11 Jun 2024 15:12:33 +0530 Rengarajan S wrote:
> >  /* define external phy id */
> >  #define      PHY_LAN8835                     (0x0007C130)
> > +#define PHY_LAN8841                  (0x00221650)
> 
> For whatever reason the existing code uses a tab between define and
> its
> name, so let's stick to that?

Sure. Will address the change in the next patch revision.

> 
> >  #define      PHY_KSZ9031RNX                  (0x00221620)
> > 
> >  /* use ethtool to change the level for any given device */
> > @@ -2327,6 +2328,13 @@ static struct phy_device
> > *lan7801_phy_init(struct lan78xx_net *dev)
> >                       netdev_err(dev->net, "Failed to register
> > fixup for PHY_LAN8835\n");
> >                       return NULL;
> >               }
> > +             /* external PHY fixup for LAN8841 */
> > +             ret = phy_register_fixup_for_uid(PHY_LAN8841,
> > 0xfffffff0,
> > +                                              lan8835_fixup);
> > +             if (ret < 0) {
> > +                     netdev_err(dev->net, "Failed to register
> > fixup for PHY_LAN8841\n");
> 
> Don't you have to unregister the previous fixup on the error path
> here?
> In fact the existing error path for PHY_LAN8835 is missing an
> unregsiter
> for PHY_KSZ9031RNX.

There is a seperate register and unregister done for PHY_LAN8835 and
PHY_KSZ9031RNX. Also, if the ret < 0 the fixup is not registered and
there is no need to unregister it further. Can you please elaborate on
what is missing in case of unregistering PHY_KSZ9031RNX.

> 
> Could you please send a separate fix for that with a Fixes tag?
> 
> > +                     return NULL;
> > +             }