From: Jackie Jone <[email protected]>
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 <[email protected]>
---
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;
}
On 6/3/2024 8:10 PM, [email protected] wrote:
> From: Jackie Jone <[email protected]>
>
> 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 <[email protected]>
> ---
> 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?
On 6/06/24 08:51, Jacob Keller wrote:
>
> On 6/3/2024 8:10 PM, [email protected] wrote:
>> From: Jackie Jone <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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.
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, [email protected] wrote:
>>> From: Jackie Jone <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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?
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, [email protected] wrote:
>>>> From: Jackie Jone <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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.
> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On
> Behalf Of Jacob Keller
> Sent: Wednesday, June 5, 2024 11:17 PM
> To: Chris Packham <[email protected]>; Jackie Jone
> <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; Nguyen,
> Anthony L <[email protected]>; intel-wired-
> [email protected]; [email protected]
> 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, [email protected] wrote:
> >>> From: Jackie Jone <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>> 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?
On Wed, Jun 05, 2024 at 01:51:24PM -0700, Jacob Keller wrote:
>
>
> On 6/3/2024 8:10 PM, [email protected] wrote:
> > From: Jackie Jone <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
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, [email protected] wrote:
>>> From: Jackie Jone <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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.
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 <[email protected]>
> 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
> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of [email protected]
> Sent: Tuesday, June 4, 2024 8:40 AM
> To: [email protected]
> Cc: [email protected]; [email protected]; [email protected]; Nguyen, Anthony L <[email protected]>; [email protected]; [email protected]; [email protected]
> Subject: [Intel-wired-lan] [PATCH] igb: Add MII write support
>
> From: Jackie Jone <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
Tested-by: Pucha Himasekhar Reddy <[email protected]> (A Contingent worker at Intel)