Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a4ecd20b-5d13-893b-2329-f6dd0c565ad5@gmail.com>
Date:   Sat, 25 Mar 2023 12:06:41 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH net-next v4 10/10] net: sunhme: Consolidate common probe
 tasks
Content-Language: en-US
To:     Simon Horman <simon.horman@corigine.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230324175136.321588-1-seanga2@gmail.com>
 <20230324175136.321588-11-seanga2@gmail.com> <ZB66hgG2+nn6CxS7@corigine.com>
From:   Sean Anderson <seanga2@gmail.com>
In-Reply-To: <ZB66hgG2+nn6CxS7@corigine.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

>> +#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;
>> +}
>> +
> 
> ...