Re: [PATCH net-next v2 5/5] net: bcmgenet: Interrogate PHY for WAKE_FILTER programming

[Jacob Keller]

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.

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?