linux-kernel - Re: [PATCH net-next] net: phy: Cleanup struct mdio_driver

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: Wed, 3 Jan 2024 14:30:20 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Yajun Deng <yajun.deng@...ux.dev>
Cc: "Russell King (Oracle)" <linux@...linux.org.uk>, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	olteanv@...il.com, hkallweit1@...il.com, kabel@...nel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-phy@...ts.infradead.org
Subject: Re: [PATCH net-next] net: phy: Cleanup struct mdio_driver_common

On Wed, Jan 03, 2024 at 07:38:04PM +0800, Yajun Deng wrote:
> 
> On 2024/1/3 18:51, Russell King (Oracle) wrote:
> > On Wed, Jan 03, 2024 at 10:03:14AM +0800, Yajun Deng wrote:
> > > On 2024/1/3 01:34, Russell King (Oracle) wrote:
> > > > I'm not sure why this consistency is even desired, the commit message
> > > > doesn't properly say _why_ this change is being proposed.
> > > Most drivers use device_driver directly. This should be added to the commit.
> > > 
> > > Like this:
> > > 
> > > struct sdio_driver {
> > > 
> > > ... ...
> > > 
> > >          struct device_driver drv;
> > > };
> > > 
> > > 
> > > struct pcie_port_service_driver {
> > > 
> > > ... ...
> > > 
> > >          struct device_driver driver;
> > > };
> > > 
> > > and so on ...
> > ... which is fine for those other drivers because they don't share the
> > same bus. That is not the case here - we have two different classes
> > of drivers on the same bus.
> 
> 
> Yes, that's true. But we can implement it with is_phy_driver(). I don't
> think we need a flag for that.
> 
> > 
> > I don't like a justification that just because other subsystems do
> > something in one particular way, that is the only way things should be
> > done. I think there is good reason to have the structure we have, and
> > thus there needs to be a good reason to change it.
> 
> Its purpose is to clean up struct mdio_driver_common, and make the code
> cleaner.

I have to agree with Russell here. The commit message should explain
the 'Why?'. Why is this better? Why is it cleaner? Why does making it
the same as all other drivers make it better, when in fact we have two
classes of devices stacked on top of it, and making it different to
every other driver actually helps developers realise that?

      Andrew