[PATCH] igb: Add MII write support

[PATCH] igb: Add MII write support

[jackie.jone]

From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>

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@alliedtelesis.co.nz>
---
 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;
 	default:
 		return -EOPNOTSUPP;
 	}

Re: [PATCH] igb: Add MII write support

[Jacob Keller]

On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote:
> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
> 
> 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@alliedtelesis.co.nz>
> ---
>  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?

Re: [PATCH] igb: Add MII write support

[Chris Packham]

On 6/06/24 08:51, Jacob Keller wrote:
>
> On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote:
>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
>>
>> 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@alliedtelesis.co.nz>
>> ---
>>   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.

Re: [PATCH] igb: Add MII write support

[Jacob Keller]

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@alliedtelesis.co.nz wrote:
>>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
>>>
>>> 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@alliedtelesis.co.nz>
>>> ---
>>>   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?

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@alliedtelesis.co.nz wrote:
>>>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
>>>>
>>>> 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@alliedtelesis.co.nz>
>>>> ---
>>>>    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.

RE: [Intel-wired-lan] [PATCH] igb: Add MII write support

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On
> Behalf Of Jacob Keller
> Sent: Wednesday, June 5, 2024 11:17 PM
> To: Chris Packham <Chris.Packham@alliedtelesis.co.nz>; Jackie Jone
> <Jackie.Jone@alliedtelesis.co.nz>; davem@davemloft.net
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Nguyen,
> Anthony L <anthony.l.nguyen@intel.com>; intel-wired-
> lan@lists.osuosl.org; kuba@kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH] igb: Add MII write support
> 
> 
> 
> 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@alliedtelesis.co.nz wrote:
> >>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
> >>>
> >>> 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@alliedtelesis.co.nz>
> >>> ---
> >>>   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 pretty sure from experience that even root applications will start cause nvmupdate issues.

> 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?

Re: [PATCH] igb: Add MII write support

[Andrew Lunn]

On Wed, Jun 05, 2024 at 01:51:24PM -0700, Jacob Keller wrote:
> 
> 
> On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote:
> > From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
> > 
> > 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@alliedtelesis.co.nz>
> > ---
> >  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?

User space can break the PHY configuration, cause the link to fail,
all behind the MAC drivers back. The generic version of this call
tries to see what registers are being written, and update state:

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy.c#L325

But you can still break 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?

Only root is allowed to use it. So it is a classic 'You have the
option to shoot yourself in the foot'.

For the use case being talked about here, there has been a few emails
one the list about implementing the IEEE 802.3 test modes. But nobody
has actually got around to doing it. Not that it would help in this
case, Intel don't use the Linux PHY drivers, which is where i would
expect to see such code implemented first. Maybe if the "Great Intel
Ethernet driver refactoring" makes progress, it could swap to using
the Linux PHY drivers.

      Andrew

Re: [PATCH] igb: Add MII write support

[Jacob Keller]

On 6/6/2024 8:41 AM, Andrew Lunn wrote:
> On Wed, Jun 05, 2024 at 01:51:24PM -0700, Jacob Keller wrote:
>>
>>
>> On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote:
>>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
>>>
>>> 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@alliedtelesis.co.nz>
>>> ---
>>>  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?
> 
> User space can break the PHY configuration, cause the link to fail,
> all behind the MAC drivers back. The generic version of this call
> tries to see what registers are being written, and update state:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy.c#L325
> 
> But you can still break it.
> 

Yea, its extremely easy to break things if you don't know what you're
doing here. So its more a question of "are we ok exposing yet another
way root can brick things?"

>> 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?
> 
> Only root is allowed to use it. So it is a classic 'You have the
> option to shoot yourself in the foot'.
> 

I don't have an objection to enabling this myself, but I do want to be
cognizant of the way it is viewed in the wider community.

> For the use case being talked about here, there has been a few emails
> one the list about implementing the IEEE 802.3 test modes. But nobody
> has actually got around to doing it. Not that it would help in this
> case, Intel don't use the Linux PHY drivers, which is where i would
> expect to see such code implemented first. Maybe if the "Great Intel
> Ethernet driver refactoring" makes progress, it could swap to using
> the Linux PHY drivers.
> 
>       Andrew

I remember this coming up several times in the past. I've always tried
to push for it, but so far unsuccessfully.

Re: [PATCH] igb: Add MII write support

[Andrew Lunn]

> Yea, its extremely easy to break things if you don't know what you're
> doing here. So its more a question of "are we ok exposing yet another
> way root can brick things?"

Many MAC drivers allow it, and we have not had complaints. It is not
really something i'm a fan of, it in theory allows user space drivers
for PHYs, but it is full of race conditions so in practice unlikely to
work reliably.

If you are worried about it causing additional support issues because
it gets abused, you could make it taint the kernel. That makes it
clear all bets are off if used. For the use case presented here, a
tainted kernel does not matter, it for lab testing, not production.

      Andrew

Re: [PATCH] igb: Add MII write support

[Jacob Keller]

On 6/6/2024 11:31 AM, Andrew Lunn wrote:
>> Yea, its extremely easy to break things if you don't know what you're
>> doing here. So its more a question of "are we ok exposing yet another
>> way root can brick things?"
> 
> Many MAC drivers allow it, and we have not had complaints. It is not
> really something i'm a fan of, it in theory allows user space drivers
> for PHYs, but it is full of race conditions so in practice unlikely to
> work reliably.
> 
> If you are worried about it causing additional support issues because
> it gets abused, you could make it taint the kernel. That makes it
> clear all bets are off if used. For the use case presented here, a
> tainted kernel does not matter, it for lab testing, not production.
> 
>       Andrew
> 

This might be a good compromise. I don't feel too strong either way
regarding tainting the kernel. Adding support here I think will overall
be more helpful than harmful. I like the tainting idea as a way to help
reduce support burden and flag that something like this was done.

That being said, I don't think I personally have a problem with the
patch as-is given its intended use case so either way from me:

Acked-by: Jacob Keller <jacob.e.keller@intel.com>

RE: [Intel-wired-lan] [PATCH] igb: Add MII write support

[Pucha, HimasekharX Reddy]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of jackie.jone@alliedtelesis.co.nz
> Sent: Tuesday, June 4, 2024 8:40 AM
> To: davem@davemloft.net
> Cc: jackie.jone@alliedtelesis.co.nz; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; chris.packham@alliedtelesis.co.nz; intel-wired-lan@lists.osuosl.org; kuba@kernel.org
> Subject: [Intel-wired-lan] [PATCH] igb: Add MII write support
>
> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz>
>
> 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@alliedtelesis.co.nz>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)