Re: [PATCH] igb: Add MII write support

[Chris Packham]

On 6/06/24 09:16, Jacob Keller wrote:
>
> On 6/5/2024 2:10 PM, Chris Packham wrote:
>> On 6/06/24 08:51, Jacob Keller wrote:
>>> On 6/3/2024 8:10 PM, jackie.jone@xxxxxxxxxxxxxxxxxxx wrote:
>>>> From: Jackie Jone <jackie.jone@xxxxxxxxxxxxxxxxxxx>
>>>>
>>>> To facilitate running PHY parametric tests, add support for the SIOCSMIIREG
>>>> ioctl. This allows a userspace application to write to the PHY registers
>>>> to enable the test modes.
>>>>
>>>> Signed-off-by: Jackie Jone <jackie.jone@xxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>>    drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>>>> index 03a4da6a1447..7fbfcf01fbf9 100644
>>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>>>> @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
>>>>    			return -EIO;
>>>>    		break;
>>>>    	case SIOCSMIIREG:
>>>> +		if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F,
>>>> +				     data->val_in))
>>>> +			return -EIO;
>>>> +		break;
>>> A handful of drivers seem to expose this. What are the consequences of
>>> exposing this ioctl? What can user space do with it?
>>>
>>> It looks like a few drivers also check something like CAP_NET_ADMIN to
>>> avoid allowing write access to all users. Is that enforced somewhere else?
>> CAP_NET_ADMIN is enforced via dev_ioctl() so it should already be
>> restricted to users with that capability.
> Ok good. That at least limits this so that random users can't cause any
> side effects.
>
> I'm not super familiar with what can be affected by writing the MII
> registers. I'm also not sure what the community thinks of exposing such
> access directly.
>
>  From the description this is intended to use for debugging and testing
> purposes?

The immediate need is to provide access to some test mode registers that 
make the PHY output specific test patterns that can be observed with an 
oscilloscope. Our hardware colleagues use these to validate new hardware 
designs. On other products we have been using those "handful of drivers" 
that already support this, this is the first design we're we've needed 
it with igb.

There is of course the alternative of exposing those test modes some 
other way but then we need to start enumerating what PHYs support which 
test modes. Some of these are defined in 802.3 but there are plenty of 
vendor extensions.

One benefit I see in this is that does allow userland access to an MII 
device. I've used it to debug non-PHY devices like the mv88e6xxx L2 
switch which has a management interface over MDIO. There's an in-kernel 
driver for this now so that specific usage isn't required but I bring it 
up as an example of a device that speaks MDIO but isn't a PHY. Whether 
this is a real advantage or not might depend on how you feel about 
userland drivers.