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

[Sean Anderson]

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@xxxxxxxxx>
>
> 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.

> > +#endif
> > +
> > +	/* Now enable the feature flags we can. */
> > +	if (hp->hm_revision == 0x20 || hp->hm_revision == 0x21)
> > +		hp->happy_flags |= HFLAG_20_21;
> > +	else if (hp->hm_revision != 0xa0)
> > +		hp->happy_flags |= HFLAG_NOT_A0;
> > +
> > +	hp->happy_block = dmam_alloc_coherent(hp->dma_dev, PAGE_SIZE,
> > +					      &hp->hblock_dvma, GFP_KERNEL);
> > +	if (!hp->happy_block)
> > +		return -ENOMEM;
> > +
> > +	/* Force check of the link first time we are brought up. */
> > +	hp->linkcheck = 0;
> > +
> > +	/* Force timer state to 'asleep' with count of zero. */
> > +	hp->timer_state = asleep;
> > +	hp->timer_ticks = 0;
> > +
> > +	timer_setup(&hp->happy_timer, happy_meal_timer, 0);
> > +
> > +	dev->netdev_ops = &hme_netdev_ops;
> > +	dev->watchdog_timeo = 5 * HZ;
> > +	dev->ethtool_ops = &hme_ethtool_ops;
> > +
> > +	/* Happy Meal can do it all... */
> > +	dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM;
> > +	dev->features |= dev->hw_features | NETIF_F_RXCSUM;
> > +
> > +
>
> nit: one blank line is enough.

Ah, oops.

--Sean

> > +	/* Grrr, Happy Meal comes up by default not advertising
> > +	 * full duplex 100baseT capabilities, fix this.
> > +	 */
> > +	spin_lock_irq(&hp->happy_lock);
> > +	happy_meal_set_initial_advertisement(hp);
> > +	spin_unlock_irq(&hp->happy_lock);
> > +
> > +	err = devm_register_netdev(hp->dma_dev, dev);
> > +	if (err)
> > +		dev_err(hp->dma_dev, "Cannot register net device, aborting.\n");
> > +	return err;
> > +}
> > +
>
> ...