Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <2ad82651-8e52-47ea-a567-2382b26f3c71@intel.com>
Date:   Fri, 27 Oct 2023 10:36:05 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v2 5/5] net: bcmgenet: Interrogate PHY for
 WAKE_FILTER programming
Content-Language: en-US
To:     Florian Fainelli <florian.fainelli@broadcom.com>,
        <netdev@vger.kernel.org>
CC:     Doug Berger <opendmb@gmail.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        "Tariq Toukan" <tariqt@nvidia.com>, Gal Pressman <gal@nvidia.com>,
        Willem de Bruijn <willemb@google.com>,
        Daniil Tatianin <d-tatianin@yandex-team.ru>,
        "Simon Horman" <horms@kernel.org>,
        Justin Chen <justin.chen@broadcom.com>,
        "Ratheesh Kannoth" <rkannoth@marvell.com>,
        Joe Damato <jdamato@fastly.com>,
        "Vincent Mailhol" <mailhol.vincent@wanadoo.fr>,
        Jiri Pirko <jiri@resnulli.us>,
        "open list" <linux-kernel@vger.kernel.org>
References: <20231026224509.112353-1-florian.fainelli@broadcom.com>
 <20231026224509.112353-6-florian.fainelli@broadcom.com>
 <0a164b9b-4f9b-4886-b19e-48298cdcff8d@intel.com>
 <2eeb8e24-4122-450b-adf5-8c8a746db518@broadcom.com>
 <6456509b-9df7-47e3-b941-c307594a80d2@intel.com>
 <93abb8d0-40c6-4758-a8de-c79d7acce6bc@broadcom.com>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <93abb8d0-40c6-4758-a8de-c79d7acce6bc@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?L3IzSlE0eitCWHVYNEhYZDFSeXVVdnVISFFLaVJ5aTRjbmtEcjZmNmUrT1hL?=
 =?utf-8?B?NmlyZ29LVGR1YzBacFV0ckZjbkxLcEVKU2Zad1hSVFZQc1czbHQ2cS91MFpE?=
 =?utf-8?B?Q0NTNFNaRXVpTi9VVU5tcnBueGtXSDVzMEJtd3MydlZ3L1ZLN1czKzMvbXFG?=
 =?utf-8?B?eWkzZmVEbUxacU15UUNWRzNadDZBUm5temFYcjJOemlUV2JOSHZ6eGhHeFM5?=
 =?utf-8?B?VTZENFNWOEZZaTZka3BDTGVsajFzSHJOTlZiNHdCbmJ4YThUWmJXbW9JTXBz?=
 =?utf-8?B?TER3SFZHRW0zSnc0VzhwUlE3aGJoYkJUZDU5SmlRUGJGaUV2TzNQNjFESVh3?=
 =?utf-8?B?cUtQY0FQZXpERCsxZmg3TDZTdnlFY2RoWitLQ2czTzkrYUp6WEM3clVGTEdw?=
 =?utf-8?B?eWZpRExkWDQzdVBiK1VEV3greFVReUZDL25QZ3Ryc1hwaUpqTGRLY3pPbkw5?=
 =?utf-8?B?djdDY2R2ZTdHMVFqQ1l4bTJmSWdKTFdBTFAvdkRScHV5ZkNoUWNWYU9aMUtK?=
 =?utf-8?B?Z1JpbWZVeGZSdmV6aUhFM2NIbWx2YWI3Qm5xNktxMkVLZ3J0ZXVWYmVOS1M4?=
 =?utf-8?B?WU1jdVFMcERTTVNoL0xLL1Q3Y2VocnNrU2E0ZFFSdFRTS2FwQ01hMm83Y3hQ?=
 =?utf-8?B?a2xKMWdCUGNQUGtJdTdtSG1WaFZDemd5RkVCTU14TzM2VXcwNCtkdGhCKzlJ?=
 =?utf-8?B?N3ZBb1hNL1FHTDlZbDJNWHg5WkJIaWZvc1VRd3czeXZOM1RvbGpGZzVSOFNU?=
 =?utf-8?B?c0JCbXpLcmR2YzU4RXlWRWZxYUV1alQzZ2Z3cGVFSzBhU21QRG1FVUsyN21N?=
 =?utf-8?B?SE41NE8rWlRreE9hL2dpVG9LQjZPdElaUjhqQzhwZFJ1N3loN1RCSFl3RDd5?=
 =?utf-8?B?T3hzRnBNVjVaTFU4N1kvQWZxM1lBZ1pvZ1MyOTE2UUl1eHN5TlVmREh4ZUow?=
 =?utf-8?B?eVMzNDdJR3dJQkIzNmF0WUpkcnZPVHJ1d2ZSWEFqcUtGU2N2bVM5MnEvVVd6?=
 =?utf-8?B?SVBHVndxeGNrNFhDVmZyM2U4THpiZzdNN1hMeTM3NXpzS2pzU21heGxiZ1kz?=
 =?utf-8?B?SDZMY2x6ZDk3R1dRSDZXRDRUQk5CbVExTXlCVSt2VzUvTXlqMGRkMGUwRFBM?=
 =?utf-8?B?czFXNlhtUmE0SWhvTnV3ZXlwWW5uTjNhdG1DSHBCSnpDRFhoQ0VNaUNCQW9k?=
 =?utf-8?B?Z0lwbEVTUjNkTmZPelJ1WndvRktoMVhPTGVFR1k4T2ZIaUlhejFUdTlpRDJB?=
 =?utf-8?B?ZUFUSmc2UnI1eURxdTVYQWJSV2FpS3BVUmVFTHlCTzUxb3A2S2VmdkhSNURV?=
 =?utf-8?B?ZHlVVXB3czVoQTV6Q0tsVEJOekVZT2hHanlNdGc1dWM4bXh5SkQyOVM5dmNz?=
 =?utf-8?B?MHQ4ZDB3SFBCcldxOEt6aElvWXp2aXJDZW42SUEvZ3RhR3dySHIrb1ZjNXFt?=
 =?utf-8?B?eDFNdTg5d1ZqZ3VWNG9qanBhUFhaZkgvM0NmYm12NGI1VjBvV3dqaFM0VWpv?=
 =?utf-8?B?OG9UOGFvVkRrZjV1UjBNaVFRbDBGVnRYbTQ2eGtlT2hKNnVmVDVqbktNTlJT?=
 =?utf-8?B?eURsRkVoenZwSVhaOTM5bm8yeERZWVVnUmhYSGRPL3F2L0ZDTm5PdjNmRlcx?=
 =?utf-8?B?Um1BanFCdXJiMnlDMFhOVkQ2YWZBQVBRTGdsd2ZHWWZqcWFSMkR5dmI3bDd5?=
 =?utf-8?B?RHd2OVE2NlUvTUJoK3F1K1djNWhPZzUwU2lEZjhJS3g5bzNodVpaTzUzUzZj?=
 =?utf-8?B?ZWtYVWJGRlQwb2VCNk9NS1FTaUpGWm5lVk5OelFwRlkzQ0IrTjZ2LzFacEpi?=
 =?utf-8?B?NXNtdkQ1bE5mT09lb2Yyd2hBYTQxYkY5Nk9GVjFubEo3UGVBdEdBTjRhYmlF?=
 =?utf-8?B?aE1LYWErMGRucGF5aTZsMjZmSi9pWThBYXpFMEcvNmUvRGdoODRiTy9EZDla?=
 =?utf-8?B?Qzk0S0lFeFFwTSt0M1MrSzY3ZDFIbWhBdGhCT25SSDhrTkhuRUNoRzI3dnlw?=
 =?utf-8?B?SVlEQ1pYRlBZbnpiUEhacER2Z0g3NU9pWG9DT0ZuZFRCQkZwQlhIbjhFSkpK?=
 =?utf-8?B?bTJMSlBnSGVET0UrVG5OZWZ3alhYeWZTQytBY1h1TXp5L3JuOXpyTHpzYUdZ?=
 =?utf-8?B?MFUzM1o1SVZ2OEIzbkVieTJhTmk3OFVmdjlha3p5WWc5TDFZcFpwRUJCazZ3?=
 =?utf-8?B?Nmc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e28bb0e5-e458-4ab5-6c2b-08dbd7133444
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Oct 2023 17:36:08.5189
 (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: WJGpcATwI6kuJcLLiI0hJ2zEEu6ovi/iOMFjAZ1P/UvlG6gjbFT3LLseTU5eqRJzTL+7s0DzMv+E5UPl2CKK9p/h5tgRET+34ETHZacaJV4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7915
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Fri, 27 Oct 2023 10:36:33 -0700 (PDT)


On 10/27/2023 10:15 AM, Florian Fainelli wrote:
> On 10/27/23 09:55, Jacob Keller wrote:
>>
>>
>> On 10/26/2023 4:52 PM, Florian Fainelli wrote:
>>> On 10/26/23 16:23, Jacob Keller wrote:
>>>>
>>>>
>>>> On 10/26/2023 3:45 PM, Florian Fainelli wrote:
>>>>> Determine whether the PHY can support waking up from the user programmed
>>>>> network filter, and if it can utilize it.
>>>>>
>>>>
>>>> Here, you're passing through to phy_ethtool_set_rxnfc, basically
>>>> allowing the lower device to program the wakeup filter if its supported. Ok.
>>>>
>>>> This almost feels like it would belong generally in the higher level
>>>> ethtool code rather than in the driver?
>>>
>>> Agreed, as Doug just pointed out to me, there is still an open question
>>> about reconciling the PHY and the MAC RXNFC spaces into a single
>>> ethtool_rxnfc structure.
>>>
>>> An ideal goal is to have zero modifications to neither the MAC or the
>>> PHY drivers such that they can both work in their own spaces as if they
>>> were alone, or combined.
>>>
>>> I suppose that if we get the number of supported rules from the MAC
>>> first, and then get the supported number of rules from the PHY next, we
>>> could do something like this:
>>>
>>> rule index
>>> | 0|
>>> | .| -> MAC rules
>>> |15|
>>> |16| -> PHY rule
>>>
>>> and each of the MAC or the PHY {get,set}_rxnfc() operate within a base
>>> rule number which is relative to their own space. So the MAC driver
>>> would continue to care about its (max..first) - base (0) range, and the
>>> PHY would care about (max..first) - base (16).
>>>
>>> Though then the issue is discoverability, how do you know which rule
>>> location is backed by which hardware block. We could create an
>>> intermediate and inert rule at index 16 for instance that acts as a
>>> delimiter?
>>>
>>> Or we could create yet another RX_CLS_LOC_* value that is "special" and
>>> can denote whether of the MAC or the PHY we should be targeting
>>> whichever is supported, but that does not usually lend itself to being
>>> logically ORed with the existing RX_CLS_LOC_* values. WDYT?
>>>
>>> pw-bot: cr
>>
>> Ah, yea there is a lot of complexity to consider here.
> 
> Yes this is only the tip of iceberg! Here is hopefully a better 
> description of our particular system where this is being requested (the 
> fact there is a single one also makes me question the entire effort, but 
> anyway). We have 2 distinct system sleep modes:
> 
> - akin to ACPI S2 where the Ethernet PHY and MAC remain enabled and both 
> can be used for Wake-on-LAN filtering, with the MAC being more capable 
> than the PHY. System power consumption is just around 500mW at the wall. 
> In that case it would make sense to leverage the MAC's capability 
> because it is better and would lead to fewer false wake-ups
> 
> - akin to ACPI S3 where the Ethernet PHY only remains enabled, the MAC 
> is powered off (as is most of the SoC), but we have limited Wake-on-LAN 
> capability in the form of network filter as we can only match on a 
> custom MAC DA + mask. System power consumption is closer to 350mW at the 
> wall.
> 
> My users are not really willing to use the broad WAKE_MCAST because they 
> want to match specifically on mDNS over IPv4 (or IPv6), so they prefer 
> to program an exact match to limit the amount of false wake-ups. 
> Arguably there will already be quite a lot in home network due to 
> phones, IoT devices, and whatnot.
> 
>  From an user perspective they would know which system standby state is 
> being entered so one could imagine that ahead of entry, we could 
> configure either the MAC, or the PHY when targeting S2, or just the PHY 
> when targeting S3. This implies that we can selectively target one 
> entity, or the other.
> 
> For the current time being, and knowing the use case of my users, 
> directing all of the Wake-on-LAN configuration towards the PHY would be 
> enough IMHO, even if that means we stop leveraging the MAC capabilities, 
> hence this patch series.
> 

Right.

>>
>> I'm not entirely sure what we should do here. What about extending with
>> another attribute entirely instead of another bit in RX_CLS_LOC?
> 
> Yes possibly, or we just target different objects, right now we have 
> visibility into the MACs via the net_device, it seems like we ought to 
> be able to target some ethtool APIs towards PHY objects, which currently 
> have no netlink representation. There is on-going work to bridge that gap:
> 
> https://lore.kernel.org/netdev/ffc6ff4a-d1af-4643-a538-fd13e6be9e06@lunn.ch/T/
> 
> but I am not sure we will reach an agreement any time soon. Maybe I can 
> convince my masters to wait for that to land and use WAKE_MCAST in the 
> meantime.
> 

Sure, but this obviously costs a potentially significant amount of extra
power, and it would be better to avoid that.

> I would not necessary want to invent a new set of ethtool commands and 
> kernel APIs such that we could do the below examples, though maybe this 
> is not incompatible with the work being done by Maxime:
> 
> # Target the Ethernet MAC
> ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 # 
> Assumes MAC by default
> ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 
> target mac
> 
> # Target the Ethernet PHY, if capable
> ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 
> target phy
> 
> # Enable WAKE_FILTER at the MAC level
> ethtool -s eth0 wol f # assumes MAC by default
> ethtool -s eth0 wol f target mac
> 
> # Enable WAKE_FILTER at the PHY level, if capable
> ethtool -s eth0 wol f target phy
> 
> though maybe this is the much needed addition to ethtool so we can be 
> more selective.
> 
> After a bunch of candies on Tuesday I might reach a state of trance and 
> figure which way to proceed :D

It does seem like an acceptable compromise here, and perhaps being
driver specific is ok, since this does depend a lot on the individual
device support, thus broadly applying this across all drivers could be
problematic.

I like the idea of being able to more precisely target the rules so that
its clear to userspace what is being done... but I also understand the
challenge of wanting to deliver what feels like a small win and being
asked to do something much larger.