Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register

["Rafał Miłecki" <zajec5@gmail.com>]

On 17.03.2021 22:20, Florian Fainelli wrote:
> On 3/17/2021 7:37 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Simple macro like REG_RGMII_CNTRL_P() is insufficient as:
>> 1. It doesn't validate port argument
>> 2. It doesn't support chipsets with non-lineral RGMII regs layout
>>
>> Missing port validation could result in getting register offset from out
>> of array. Random memory -> random offset -> random reads/writes. It
>> affected e.g. BCM4908 for REG_RGMII_CNTRL_P(7).
> 
> That is entirely fair, however as a bug fix this is not necessarily the
> simplest way to approach this.

I'm not sure if I understand. Should I fix it in some totally different
way? Or should I just follow your inline suggestions?


>> Fixes: a78e86ed586d ("net: dsa: bcm_sf2: Prepare for different register layouts")
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>   drivers/net/dsa/bcm_sf2.c      | 51 ++++++++++++++++++++++++++++++----
>>   drivers/net/dsa/bcm_sf2_regs.h |  2 --
>>   2 files changed, 45 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
>> index ba5d546d06aa..942773bcb7e0 100644
>> --- a/drivers/net/dsa/bcm_sf2.c
>> +++ b/drivers/net/dsa/bcm_sf2.c
>> @@ -32,6 +32,30 @@
>>   #include "b53/b53_priv.h"
>>   #include "b53/b53_regs.h"
>>   
>> +static u16 bcm_sf2_reg_rgmii_cntrl(struct bcm_sf2_priv *priv, int port)
> 
> This is not meant to be used outside the file, so I would be keen on
> removing the bcm_sf2_ prefix to make the name shorter and closer to the
> original macro name.

Most or all local functions use such a prefix. E.g.:
bcm_sf2_num_active_ports()
bcm_sf2_recalc_clock()
bcm_sf2_imp_setup()
bcm_sf2_gphy_enable_set()
bcm_sf2_port_intr_enable()
bcm_sf2_port_intr_disable()
bcm_sf2_port_setup()
bcm_sf2_port_disable()

It would be inconsistent to have RGMII reg function not follow that.


>> +{
>> +	switch (priv->type) {
>> +	case BCM4908_DEVICE_ID:
>> +		/* TODO */
>> +		break;
>> +	default:
>> +		switch (port) {
>> +		case 0:
>> +			return REG_RGMII_0_CNTRL;
>> +		case 1:
>> +			return REG_RGMII_1_CNTRL;
>> +		case 2:
>> +			return REG_RGMII_2_CNTRL;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
>> +	WARN_ONCE(1, "Unsupported port %d\n", port);
>> +
>> +	return 0;
> 
> maybe return -1 or -EINVAL just in case 0 happens to be a valid offset
> in the future. Checking the return value is not necessarily going to be
> helpful as it needs immediate fixing, so what we could do is keep the
> WARN_ON, and return the offset of REG_SWITCH_STATUS which is a read-only
> register. This will trigger the bus arbiter logic to return an error
> because a write was attempted from a read-only register.
> 
> What do you think?

Great, thanks!