linux-kernel - Re: [net-next PATCH 06/14] net: phy: at803x: move at8031 specific data out of generic at803x

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: <568f8b22-a7d2-46c3-a539-30ecf6a85b18@lunn.ch>
Date:   Thu, 30 Nov 2023 21:14:00 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [net-next PATCH 06/14] net: phy: at803x: move at8031 specific
 data out of generic at803x_priv

On Thu, Nov 30, 2023 at 08:38:17PM +0100, Christian Marangi wrote:
> On Thu, Nov 30, 2023 at 04:21:50PM +0100, Andrew Lunn wrote:
> > > +struct at8031_data {
> > > +	bool is_fiber;
> > > +	bool is_1000basex;
> > > +	struct regulator_dev *vddio_rdev;
> > > +	struct regulator_dev *vddh_rdev;
> > > +};
> > > +
> > >  struct at803x_priv {
> > >  	int flags;
> > >  	u16 clk_25m_reg;
> > >  	u16 clk_25m_mask;
> > >  	u8 smarteee_lpi_tw_1g;
> > >  	u8 smarteee_lpi_tw_100m;
> > > -	bool is_fiber;
> > > -	bool is_1000basex;
> > > -	struct regulator_dev *vddio_rdev;
> > > -	struct regulator_dev *vddh_rdev;
> > > +
> > > +	/* Specific data for at8031 PHYs */
> > > +	void *data;
> > >  };
> > 
> > I don't really like this void *
> > 
> > Go through at803x_priv and find out what is common to them all, and
> > keep that in one structure. Add per family private structures which
> > include the common as a member.
> 
> As you notice later in the patches, only at803x have stuff in common
> qca803xx and qca808x doesn't use the struct at all (aside from stats)

The dangers here are taking a phydev->priv and casting it. You think
it is X, but is actually Y, and bad things happen.

The helpers you have in your common.c must never do this. You can have
a at803x_priv only visible inside the at803x driver, and a
qca808x_priv only visible inside the qca808x driver. Define a
structure which is needed for the shared code in common.c, and pass it
as a parameter to these helpers.

You have a reasonably good idea what your end goal is. The tricky part
is getting there, in lots of easy to review, obviously correct steps.

	Andrew