linux-kernel - Re: [PATCH net-next v4 10/10] net: sunhme: Consolidate common probe tasks

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: <ZB/63cE1DtYNpgtV@corigine.com>
Date:   Sun, 26 Mar 2023 09:57:17 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Sean Anderson <seanga2@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v4 10/10] net: sunhme: Consolidate common probe
 tasks

On Sat, Mar 25, 2023 at 12:06:41PM -0400, Sean Anderson wrote:
> On 3/25/23 05:10, Simon Horman wrote:
> > On Fri, Mar 24, 2023 at 01:51:36PM -0400, Sean Anderson wrote:
> > > Most of the second half of the PCI/SBUS probe functions are the same.
> > > Consolidate them into a common function.
> > > 
> > > Signed-off-by: Sean Anderson <seanga2@...il.com>
> > 
> > Hi Sean,
> > 
> > overall this looks good.
> > But I (still?) have some concerns about handling hm_revision.
> > 
> > ...
> > 
> > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> > > index bd1925f575c4..ec85aef35bf9 100644
> > > --- a/drivers/net/ethernet/sun/sunhme.c
> > > +++ b/drivers/net/ethernet/sun/sunhme.c
> > > @@ -2430,6 +2430,58 @@ static void happy_meal_addr_init(struct happy_meal *hp,
> > >   	}
> > >   }
> > > +static int happy_meal_common_probe(struct happy_meal *hp,
> > > +				   struct device_node *dp)
> > > +{
> > > +	struct net_device *dev = hp->dev;
> > > +	int err;
> > > +
> > > +#ifdef CONFIG_SPARC
> > > +	hp->hm_revision = of_getintprop_default(dp, "hm-rev", hp->hm_revision);
> > 
> > Previously the logic, for SPARC for PCI went something like this:
> > 
> > 	/* in happy_meal_pci_probe() */
> > 	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> > 	if (hp->hm_revision == 0xff)
> > 		hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
> > 
> > Now it goes something like this:
> > 
> > 	/* in happy_meal_pci_probe() */
> > 	hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
> > 	/* in happy_meal_common_probe() */
> > 	hp->hm_revision = of_getintprop_default(dp, "hm-rev", hp->hm_revision);
> > 
> > Is this intentional?
> > 
> > Likewise, for sbus (which implies SPARC) the logic was something like:
> > 
> > 	/* in happy_meal_sbus_probe_one() */
> > 	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> > 	if (hp->hm_revision == 0xff)
> > 		 hp->hm_revision = 0xa0;
> > 
> > And now goes something like this:
> > 
> > 	/* in happy_meal_pci_probe() */
> > 	hp->hm_revision = 0xa0;
> > 	/* in happy_meal_common_probe() */
> > 	hp->hm_revision = of_getintprop_default(dp, "hm-rev", hp->hm_revision);
> 
> Yes, this is intentional. Logically, they are the same; we just set up the default
> before calling of_getintprop_default instead of after.

Thanks, I see that now :)

And I have no further questions about this patch.

Reviewed-by: Simon Horman <simon.horman@...igine.com>