Received-SPF: pass (google.com: domain of linux-kernel+bounces-203241-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <4f9af0e9-5ce0-4b76-a2cd-cbd37331d869@intel.com>
Date: Wed, 5 Jun 2024 14:16:47 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] igb: Add MII write support
To: Chris Packham <Chris.Packham@alliedtelesis.co.nz>, Jackie Jone
	<Jackie.Jone@alliedtelesis.co.nz>, "davem@davemloft.net"
	<davem@davemloft.net>
CC: "jesse.brandeburg@intel.com" <jesse.brandeburg@intel.com>,
	"anthony.l.nguyen@intel.com" <anthony.l.nguyen@intel.com>, "kuba@kernel.org"
	<kuba@kernel.org>, "intel-wired-lan@lists.osuosl.org"
	<intel-wired-lan@lists.osuosl.org>, "netdev@vger.kernel.org"
	<netdev@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
References: <20240604031020.2313175-1-jackie.jone@alliedtelesis.co.nz>
 <ad56235d-d267-4477-9c35-210309286ff4@intel.com>
 <dce11b71-724c-4c5f-bc95-1b59e7cc7844@alliedtelesis.co.nz>
Content-Language: en-US
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <dce11b71-724c-4c5f-bc95-1b59e7cc7844@alliedtelesis.co.nz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ejE0azZPTDM5UTgyS2RaMHFXdnJVVEFuNUUxRnA5ZmltcVhOa1huK1dWSE1u?=
 =?utf-8?B?TkRKcXdNa2I2ZTlIVGZYRiszQlMwTDJBU1hLQlM3Wm5hL2srUHFXK1h6clEv?=
 =?utf-8?B?NWhrRWJKcm1pNDNXS3ZZUTdmSzhmWkxrQ0xyNmwyYy9Ddi9Obm43aS9nNkdt?=
 =?utf-8?B?bHErTGF1d3l1Wk9aRlJSbk45RVh2aUorWjRJNXRNeFFySUY5SVJPdDRUL0kx?=
 =?utf-8?B?OTlJVVhaUkswbG1NWVpTR0hnSWdBRS81R1Rkd0hyQmJHZjltUytLZXdobXBF?=
 =?utf-8?B?M0RNYjg1UGFHWVJZblJHUjZ2aURWUFA2eGNXU1VsUmdiY0c3TGw3aXNibWVZ?=
 =?utf-8?B?a2ZKNXBSMHI0ZEl0Sm9YZitmY3RyTW9QTGx6UGFmNzVLQXU5OU1UTjJRM3A2?=
 =?utf-8?B?UTVHRXNTekl3T0c1MC95TFBYWGFMSkQzY0JGY2ZzYTlIN0cxQks4WjVPT2lK?=
 =?utf-8?B?OU9VT05wRFhvMjVlam1HbW1PUzVoQSsxSnhqZUJ5OVpDanhhaUhEMVVvTTVV?=
 =?utf-8?B?dXZ6RWFXQS9ibC84UHArcnJVNVBBejA0OG1ydmFPMC9tSC93Zkh5R0phdDNX?=
 =?utf-8?B?Sm1XaHd3VUdLZFJia3hRZER3R3dOZnNuNjVNSW43NnkzNE1jbGpYOFdoSFEv?=
 =?utf-8?B?NGlRUGprZWo4MjI3ZHIySHhlZlMyNGx6RFM1NmJPZ1VJUGNHUCtKZ0Z5ZEdX?=
 =?utf-8?B?dHQvS1FqS1gvTStBeGFHUDJnYWxZSWNadkM0MldMOS9nZWJXSU5MNHBxMlpI?=
 =?utf-8?B?NnVKTUNUZ21jTHJycFNwQzhWNGtKNlpVMHZDUWxPTmQwVDBLQkNueDVlaHVj?=
 =?utf-8?B?aUs5Y1M4V0t4aW1JVnVuU1VBekFJNytsVjI1ZXZOeXdaeUtLSUVRcGlGdXZl?=
 =?utf-8?B?eEIwY3RtUkpzMWJMMU9XaU5DWVBhY1dlbnB6dDNiQ08vVTlkVU5uc3dlZEUy?=
 =?utf-8?B?N1czWUdWQXJTZFpsaGRwTUhCQ1lZUENjbmJrQU9nd3pZckcyTTVocHAxNVR6?=
 =?utf-8?B?ZGhKQ0pob1E0Uk50aGdkYTRNb2VQMHZZK25MUk1TRlJ2Q1RDMDFGVytiREpY?=
 =?utf-8?B?VStkUk82MVhTMlo4eXpqZnVaQk9ja3BXVWYzM25JMDBxYi9TdkdIeWpMc0hh?=
 =?utf-8?B?MFVCM1RKN0xMaUFCeXVUNEVYRzV1VFFmbjBhekZhRlJ4Snk2aytuajRPU05n?=
 =?utf-8?B?VUZKTjJiNTAzOHhqNS9kL1dWNU5aY2ZqN2p6T3NCZUlSQmUvNkkxMmg2Zjly?=
 =?utf-8?B?M1lwTHl5cVVFbzNjMm5RUnBCNkVmdGlKNkVwS0puMEdaZjJvN2RTUUU5MU8y?=
 =?utf-8?B?VGIyOXprTGNNcm1VSy93cERoNFlJVUNINGt6cFg0SERKRXRGb1ZOYkUra2cv?=
 =?utf-8?B?bmhVV3B5bzh0TGduVUZXNVFuUG9uZWtzdGhldlFHWE54OWFYRjV5R0FoSUFN?=
 =?utf-8?B?SFBOSmV0RU9iMDdOZldSaE9aV1VzRDgra05vT0tsR3dwUEhiU3pzZ0xMQ2hO?=
 =?utf-8?B?QlJDUi9ZbFZqbHJLKzZnQ3BZRGJ2bjRiSTZMbVVUWUVwLzRNZ3FEQTNMSWh1?=
 =?utf-8?B?clFYVDhZS2NEWk9aQWdqc0t0eGhMZmx0NVB2Tld4Z2hYMzkrci9UTTc5d0U3?=
 =?utf-8?B?cTdoSzJ2NWN5Uk13OHY1SmY3bzNLS1B1czVVUDVDNTB0MFQwaENMNTcwVVBq?=
 =?utf-8?B?bjVHUEsxaW5IVDlRcjNHUkd1OGNXNm1tdlZvQmc2bDR5T2I5ajROY1lyQWps?=
 =?utf-8?B?VUgzb0VjOFF1WnU2UFhyYU9kd3prWUZFK2Fub2lGQTE5M09uSVNKejhHcWFh?=
 =?utf-8?B?bFN1UFdvREVHbUpScFNucVlpd1AzSXVxNkY4RmZ2NmQ3TWl3UVFuYlNHOEVz?=
 =?utf-8?B?MXdFcTVFV1FEd094TDRrN0ZmRDdpZlJiVHFZdDNZNXFnYTdteElKNXFqQkFs?=
 =?utf-8?B?NzRVSW5zY2pFMFpyWXI0a1VsN3hHODI3aWRZOHQzYUJJdVZ2M3M5dXg0RTJy?=
 =?utf-8?B?YW9jVkhSc1JoSHZ3eTdpV3hUNDNhWm9PZG52Q3I4M0RBSFhmZWxZWnAzUU5k?=
 =?utf-8?B?MCs0WEcxNjdvUjZOaEJWQlk2MTU4Rkhmb0xKZCtVSCtWNERGZk1LV2dZdjkw?=
 =?utf-8?B?VkpNM0tUTVBMV0NNSDhSY05GZVFxaE9KcnpzMlM3a3NjNlhHcG9DYldOTjVC?=
 =?utf-8?B?MXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fd9c2980-0fcb-4342-d045-08dc85a4cffc
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jun 2024 21:16:49.1072
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: MTOjkdvTc/zydyEU67zE7q2PT4qtRg4KJv1nVNC0qQishylw9sOdPn6QHxOxoEfDyn1Ir/0o5NF1T7yJPL4gMP53iCDEGzYl6f2DUfjCawg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6375
X-OriginatorOrg: intel.com


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?