Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sean Anderson <sean.anderson@seco.com>
Subject: Re: [RFC net-next PATCH 05/16] net: phylink: Automatically attach PCS
 devices
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     netdev@vger.kernel.org, "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Saravana Kannan <saravanak@google.com>
References: <20211004191527.1610759-1-sean.anderson@seco.com>
 <20211004191527.1610759-6-sean.anderson@seco.com>
 <YVwfWiMOQH0U5bay@shell.armlinux.org.uk>
 <61147f21-6de4-d91e-c16f-fdb539e52b42@seco.com>
 <YV7Kp2k8VvN7J0fY@shell.armlinux.org.uk>
Message-ID: <9f73bc4f-5f99-95f5-78fa-dac96f9e0146@seco.com>
Date:   Thu, 7 Oct 2021 20:14:35 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
In-Reply-To: <YV7Kp2k8VvN7J0fY@shell.armlinux.org.uk>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SUxjQzdCMkRTUFNzcFFCbGVDTmxhczNxRmphTldaVVhUWHRFT29NYnN4cTIy?=
 =?utf-8?B?bzNZYmFsUjkrcmM5Myt4NE4xaVNIVDM1SVRxVDZ0U2RGNXBnODlzZ3NXUEtO?=
 =?utf-8?B?ZlNBajV1b2lLVFhnNzhHMktDSXF3L2xKSko1YU96SFdQRURZeFg5Z3JQR1FG?=
 =?utf-8?B?STJ6SnZYWVpoMHZ2RWtKUlhiUnJnbk4rU2lOazNac0R0QjdKUVB0M1BkTHI5?=
 =?utf-8?B?SHpkbEtQNWZHMG9Sa2tPeVhLdWJnaitzODBuZlNIQ01MVjdFMUwzcnNVMDN1?=
 =?utf-8?B?b3NscmhqeEtTNjVUaTFYbktoL2YrVE02SmlXYzFhMVMyR2Rxc3pHaU1wWHlT?=
 =?utf-8?B?K0NwUmVaZldOZEpZTVVQam8xZHROZWlmWHVSdHBkYWpqS1oxcnR5Ylc4MFBC?=
 =?utf-8?B?SjVrRUN6VTBRQjBIVjFBWHNmV0RFckFjZlU3M1NYMnhmUWVqMEo0eEdHK2lo?=
 =?utf-8?B?cnhDZXduWUIvR084Q3FvTEdvbDltVjlaRlhhM25uam01cEtRZm1Ca1BZZWxW?=
 =?utf-8?B?eDJrRE1xZUJNTmNmRU9oMVdhajRwcys5d1UzcFhOc0V6Y3p4anFXdnI3NE5C?=
 =?utf-8?B?cXBoZk1Ca0JUd1hoZm5oNDI3M2x1YlNYZHlwb1dTV05iMlRNNkRWU1EzTE82?=
 =?utf-8?B?ZWt2eUJDendJaHRzUTM4N0htNDRSREtOV2JiS3BNWmdTamkrZURqRitSZ0FF?=
 =?utf-8?B?anVKR25PSFNRWVBkb1FTV2hIZTN2a1hEYW5KekF2R0VjZXRMaU5HWUx3RjRE?=
 =?utf-8?B?WDZob0w3U0dPNFJ3dExWOVRSNjdBVW9mdFVZWW5tbU15a2k5a0x5clJlOVhu?=
 =?utf-8?B?WGdTcGt4dHZSdTJhMEZ2akc1aGpwMnlBandqUlF1eU5xVE4rZHN2ZkZNN1BO?=
 =?utf-8?B?NVpEbURNejUzcFUvaFN6ZGhnTTdoRjh1VGgrU1pjRjVib1FIV2V1b29uV1lj?=
 =?utf-8?B?U1o1MlN3ckpUWlNENmpvVjZHNmg2M1RUSFM1WHFqTSsyMUkweVNCUzRYaElx?=
 =?utf-8?B?WHBYSFpIaDRNSmR4bVBxcUMwTW1pWFVmcnJ6ZDAxRWNTMEZUWGc2aUJlUm9T?=
 =?utf-8?B?eTlQaWhjZENxU2VaVlErcGxlamtPTUx1TDFzSkVwNmVQcjU1ZEpZQ0RyUnFH?=
 =?utf-8?B?eFN3TkxyeGs0YXM3Q3F5LzdzWmpxV1ViQlBSb3dzdUNYMmdZRzcwRU9nVzJs?=
 =?utf-8?B?ZVFwV0JvY003NGNqYjErd0Y3SkxiL0Mvd0xPajhvazRwdGlCbDF6OHVjcGJK?=
 =?utf-8?B?dldGMy9oR0Q3cjM4K0ZpaEhRM1VOWVAvWS9nczlIYThrK1lEVjZnYjhIQmpS?=
 =?utf-8?B?UkhTQlJLcGh1NlgzUVhKZzd5MkFJNUdrM3QwVlVPWEM0OGNNN3NIblJzYTV1?=
 =?utf-8?B?R1dVR3FuWmFYbXo1L1BjZ1ZQbSt1UU1LS1oxSjRXQ1VQSkRHbjZVQ0R6OXcw?=
 =?utf-8?B?NGt1b252dDdBdHRSV1Zlc25qdGRBR3FwZUhTckhEVXFXNnhxU1RicmJwcFRi?=
 =?utf-8?B?S1B4STNmMmw0WFZPVm1WQW9VK0Q0OFh1MWoxQ3JZcm90elhld2JnTWQ2UU1r?=
 =?utf-8?B?WlhiWENwTXZrUjFQbStiemRFa2pkalUzOG50SXptVlFoa0VTYUdZeGd1SUM4?=
 =?utf-8?B?ZEFBQkFsbXZCYy91cTRKak5aRHVUVVl5RGU4OHVDU0wwc1dwdDUrc25QUlhP?=
 =?utf-8?B?MVk0R1N2eEsxUXlFcENzdUpGemtwSzNtejdpU2dwZ0ZDbjIvaW85dGFkR1RL?=
 =?utf-8?Q?RCiExsddmE+Wirv01Jke1Jl3UdG274qhDpMKsAK?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 63885bf2-9681-4bca-83d8-08d989f0a2a7
X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4523.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Oct 2021 00:14:46.5648
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: GGlkbQla9Tcan5Etheqt1dNBLwPCvzJRbjacw9bJTTAGMB//3IiBfJXD27X1JxV4+sfl7Hjcp5q35h1E9kXXOA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR03MB5366
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/7/21 6:23 AM, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 12:42:53PM -0400, Sean Anderson wrote:
>>
>>
>> On 10/5/21 5:48 AM, Russell King (Oracle) wrote:
>> > On Mon, Oct 04, 2021 at 03:15:16PM -0400, Sean Anderson wrote:
>> > > This adds support for automatically attaching PCS devices when creating
>> > > a phylink. To do this, drivers must first register with
>> > > phylink_register_pcs. After that, new phylinks will attach the PCS
>> > > device specified by the "pcs" property.
>> > >
>> > > At the moment there is no support for specifying the interface used to
>> > > talk to the PCS. The MAC driver is expected to know how to talk to the
>> > > PCS. This is not a change, but it is perhaps an area for improvement.
>> > >
>> > > I believe this is mostly correct with regard to registering/
>> > > unregistering. However I am not too familiar with the guts of Linux's
>> > > device subsystem. It is possible (likely, even) that the current system
>> > > is insufficient to prevent removing PCS devices which are still in-use.
>> > > I would really appreciate any feedback, or suggestions of subsystems to
>> > > use as reference. In particular: do I need to manually create device
>> > > links? Should I instead add an entry to of_supplier_bindings? Do I need
>> > > a call to try_module_get?
>> >
>> > I think this is an area that needs to be thought about carefully.
>> > Things are not trivial here.
>> >
>> > The first mistake I see below is the use of device links. pl->dev is
>> > the "struct device" embedded within "struct net_device". This doesn't
>> > have a driver associated with it, and so using device links is likely
>> > ineffectual.

Ok, so the 'real' device is actually the parent of pl->netdev->dev?

>>
>> So what can the device in net_device be used for?
>
> That is used for the class device that is commonly found in
> /sys/devices/$pathtothedevice/net/$interfacename

By the way, why don't we set pl->dev = config->dev->parent in
phylink_create() when config->type == PHYLINK_NETDEV?

>> > Even with the right device, I think careful thought is needed - we have
>> > network drivers where one "struct device" contains multiple network
>> > interfaces. Should the removal of a PCS from one network interface take
>> > out all of them?
>>
>> Well, it's more of the other way around. We need to prevent removing the
>> PCS while it is still in-use.
>
> devlinks don't do that - if the "producer" device goes away, they force
> the "consumer" device to be unbound.

Ah, I didn't realize that was the relationship being modeled.

> As I mention above, the "consumer" device, which would be the device
> providing the network interface(s) could have more than one interface
> and unbinding it could have drastic consequences for the platform.

Well, then don't unbind the PCS ;)

After reviewing several other subsystems, I think the correct way to
approach this is to add an entry to of_supplier_bindings, which will
help out with ordering, and get the module when looking up the PCS. That
is, something like

int phylink_get_pcs(fwnode, struct phylink_pcs **pcs)
{
	int ret;
	struct fwnode_reference_args ref;

	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle", NULL,
						 0, 0, &ref);
	if (ret)
		return ret;

	*pcs = phylink_find_pcs(ref.fwnode);
	fwnode_handle_put(ref.fwnode);
	if (!*pcs)
		return -EPROBE_DEFER;

	if (!try_module_get(*pcs->owner))
		return -EBUSY;
	return 0;
}

phylink_put_pcs(pcs)
{
	module_put(pcs->owner);
}

and keep phylink_set as-is (the above should be considered along with my comments on patch 10).

Realistically, the only time a PCS is optional is if there isn't a PCS
reference in the device tree.

>> > Alternatively, could we instead use phylink to "unplug" the PCS and
>> > mark the link down - would that be a better approach than trying to
>> > use device links?
>>
>> So here, I think the logic should be: allow phylink to "unplug" the PCS
>> only when the link is down.
>
> When a device is unbound from its driver, the driver has no say in
> whether that goes ahead or not. Think about it as grabbing that USB
> stick plugged into your computer and you yanking it out. None of the
> software gets a look in to say "don't do that".

I suspect the vast majority of PCSs will be DEVICE_FIXED.

> phylink (or any other subsystem) does not have the power to say
> "I don't want XYZ to be removed".

However, we do have the power to say "I don't want XYZ's module to be
removed", which should cover most of the situations where a device is
removed after boot.

> Yes, it's harder to do that with PCS, but my point is that if one asks
> the driver model to unbind the PCS driver from the PCS device, then
> the driver model will do that whether the PCS driver wants to allow it
> at that moment or not. It isn't something the PCS driver can prevent.
>
> One can tell the driver model not to expose the bind/unbind attributes
> for the driver, but that doesn't stop the unbind happening should the
> struct device actually go away.
>
> So, IMHO, it's better to design assuming that components will go away
> at an inconvenient time and deal with it gracefully.

See above, but I think it's better here to assume that components will
stick around and if they disappear at an inconvenient time then we
should just let the netdev be removed as well.

--Sean