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: <7367681a-3ef5-42ad-9c2f-173f77cc1b56@lunn.ch>
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ