| Message ID | 20240130104845.3995341-3-vineeth.karumanchi@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: macb: WOL enhancements | expand |
On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote: > "wol-arp-packet" property enables WOL with ARP packet. > It is an extension to "magic-packet for WOL. If it is an extension to "magic-packet" why does it not depend on "magic-packet"? Are there systems that would only support the magic arp packet but a regular magic packet? > > Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> > --- > 7c4a1d0cfdc1 net: macb: make magic-packet property generic > which added magic-property support and wol-arp-packet addition > is similar extension. > --- > Documentation/devicetree/bindings/net/cdns,macb.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml > index bf8894a0257e..4bea177e85bc 100644 > --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml > +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml > @@ -144,6 +144,11 @@ patternProperties: > description: > Indicates that the hardware supports waking up via magic packet. > > + wol-arp-packet: Bikeshedding perhaps, but why not call it "magic-arp-packet" if it has the same function as the other property here? Thanks, Conor. > + type: boolean > + description: > + Indicates that the hardware supports waking up via ARP packet. > + > unevaluatedProperties: false > > required: > -- > 2.34.1 >
On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote: > "wol-arp-packet" property enables WOL with ARP packet. > It is an extension to "magic-packet for WOL. It not clear why this is needed. Is this not a standard feature of the IP? Is there no hardware bit indicating the capability? Andrew
Hi Conor, On 30/01/24 11:00 pm, Conor Dooley wrote: > On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote: >> "wol-arp-packet" property enables WOL with ARP packet. >> It is an extension to "magic-packet for WOL. > > If it is an extension to "magic-packet" why does it not depend on > "magic-packet"? Are there systems that would only support the magic arp > packet but a regular magic packet? > The IP version on ZU+ and Versal supports the below combinations for WOL event: 1. Magic packet (Wake-on magic packet only) 2. ARP (Wake-on ARP packet only) 3. Magic packet or ARP (Wake-on magic or ARP packets) The existing DT binding already has one entry for wol via magic packet. We are adding ARP packet support to the existing implementation. I will change the commit message in v2. >> >> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> >> --- >> 7c4a1d0cfdc1 net: macb: make magic-packet property generic >> which added magic-property support and wol-arp-packet addition >> is similar extension. >> --- >> Documentation/devicetree/bindings/net/cdns,macb.yaml | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml >> index bf8894a0257e..4bea177e85bc 100644 >> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml >> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml >> @@ -144,6 +144,11 @@ patternProperties: >> description: >> Indicates that the hardware supports waking up via magic packet. >> >> + wol-arp-packet: > > Bikeshedding perhaps, but why not call it "magic-arp-packet" if it has > the same function as the other property here? > Magic packet and ARP packets are two different wol events. IP supports configuring in the above-mentioned ways. Hence, I think it would be good to not mix with magic packet. Please let me know your suggestions/comments. Thanks, Vineeth
Hi Andrew, On 31/01/24 6:56 am, Andrew Lunn wrote: > On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote: >> "wol-arp-packet" property enables WOL with ARP packet. >> It is an extension to "magic-packet for WOL. > > It not clear why this is needed. Is this not a standard feature of the > IP? Is there no hardware bit indicating the capability? > WOL via both ARP and Magic packet is supported by the IP version on ZU+ and Versal. However, user can choose which type of packet to recognize as a WOL event - magic packet or ARP. The existing DT binding already describes one entry for wol via magic packet. Hence, adding a new packet type using the same methodology. vineeth > Andrew
On 31/01/2024 08:39, Vineeth Karumanchi wrote: > Hi Andrew, > > > On 31/01/24 6:56 am, Andrew Lunn wrote: >> On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote: >>> "wol-arp-packet" property enables WOL with ARP packet. >>> It is an extension to "magic-packet for WOL. >> >> It not clear why this is needed. Is this not a standard feature of the >> IP? Is there no hardware bit indicating the capability? >> > > WOL via both ARP and Magic packet is supported by the IP version on ZU+ > and Versal. However, user can choose which type of packet to recognize > as a WOL event - magic packet or ARP. The existing DT binding already > describes one entry for wol via magic packet. Hence, adding a new packet > type using the same methodology. And why would this be board-level configuration? This looks like OS policy. Best regards, Krzysztof
On Wed, Jan 31, 2024 at 01:09:19PM +0530, Vineeth Karumanchi wrote: > Hi Andrew, > > > On 31/01/24 6:56 am, Andrew Lunn wrote: > > On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote: > > > "wol-arp-packet" property enables WOL with ARP packet. > > > It is an extension to "magic-packet for WOL. > > > > It not clear why this is needed. Is this not a standard feature of the > > IP? Is there no hardware bit indicating the capability? > > > > WOL via both ARP and Magic packet is supported by the IP version on ZU+ and > Versal. However, user can choose which type of packet to recognize as a WOL > event - magic packet or ARP. ethtool says: wol p|u|m|b|a|g|s|f|d... Sets Wake-on-LAN options. Not all devices support this. The argument to this option is a string of characters specifying which options to enable. p Wake on PHY activity u Wake on unicast messages m Wake on multicast messages b Wake on broadcast messages a Wake on ARP g Wake on MagicPacket™ s Enable SecureOn™ password for MagicPacket™ f Wake on filter(s) d Disable (wake on nothing). This option clears all previous options. So why do we need a DT property? Andrew
Hi Andrew, Krzysztof, On 31/01/24 6:48 pm, Andrew Lunn wrote: > On Wed, Jan 31, 2024 at 01:09:19PM +0530, Vineeth Karumanchi wrote: >> Hi Andrew, >> >> >> On 31/01/24 6:56 am, Andrew Lunn wrote: >>> On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote: >>>> "wol-arp-packet" property enables WOL with ARP packet. >>>> It is an extension to "magic-packet for WOL. >>> >>> It not clear why this is needed. Is this not a standard feature of the >>> IP? Is there no hardware bit indicating the capability? >>> >> >> WOL via both ARP and Magic packet is supported by the IP version on ZU+ and >> Versal. However, user can choose which type of packet to recognize as a WOL >> event - magic packet or ARP. > > ethtool says: > > wol p|u|m|b|a|g|s|f|d... > Sets Wake-on-LAN options. Not all devices support this. The argument to this option is a > string of characters specifying which options to enable. > p Wake on PHY activity > u Wake on unicast messages > m Wake on multicast messages > b Wake on broadcast messages > a Wake on ARP > g Wake on MagicPacket™ > s Enable SecureOn™ password for MagicPacket™ > f Wake on filter(s) > d Disable (wake on nothing). This option > clears all previous options. > > So why do we need a DT property? > The earlier implementation of WOL (magic-packet) was using DT property. We added one more packet type using DT property to be in-line with the earlier implementation. However, I echo with you that this feature should be in driver (CAPS). We will re-work the implementation with the below flow: - Add MACB_CAPS_WOL capability to the supported platforms - Advertise supported WOL packet types based on the CAPS in ethtool. - Users can set packet type using ethtool. Please let me know your thoughts/suggestions.
On Thu, Feb 01, 2024 at 12:11:15PM +0530, Vineeth Karumanchi wrote: > Hi Andrew, Krzysztof, > > > > On 31/01/24 6:48 pm, Andrew Lunn wrote: > > On Wed, Jan 31, 2024 at 01:09:19PM +0530, Vineeth Karumanchi wrote: > > > Hi Andrew, > > > > > > > > > On 31/01/24 6:56 am, Andrew Lunn wrote: > > > > On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote: > > > > > "wol-arp-packet" property enables WOL with ARP packet. > > > > > It is an extension to "magic-packet for WOL. > > > > > > > > It not clear why this is needed. Is this not a standard feature of the > > > > IP? Is there no hardware bit indicating the capability? > > > > > > > > > > WOL via both ARP and Magic packet is supported by the IP version on ZU+ and > > > Versal. However, user can choose which type of packet to recognize as a WOL > > > event - magic packet or ARP. > > > > ethtool says: > > > > wol p|u|m|b|a|g|s|f|d... > > Sets Wake-on-LAN options. Not all devices support this. The argument to this option is a > > string of characters specifying which options to enable. > > p Wake on PHY activity > > u Wake on unicast messages > > m Wake on multicast messages > > b Wake on broadcast messages > > a Wake on ARP > > g Wake on MagicPacket™ > > s Enable SecureOn™ password for MagicPacket™ > > f Wake on filter(s) > > d Disable (wake on nothing). This option > > clears all previous options. > > > > So why do we need a DT property? > > > > The earlier implementation of WOL (magic-packet) was using DT property. > We added one more packet type using DT property to be in-line with the > earlier implementation. I can understand that. It also suggests we did a bad job reviewing that patch, and should of rejected it. But it was added a long time ago, and we were less strict back then. > > However, I echo with you that this feature should be in driver (CAPS). > We will re-work the implementation with the below flow: > > - Add MACB_CAPS_WOL capability to the supported platforms > - Advertise supported WOL packet types based on the CAPS in ethtool. > - Users can set packet type using ethtool. Yes, this sounds good. Maybe add to that, mark magic-packet deprecated, and a comment that ethtool should be used instead. Thanks Andrew
Hi Andrew, On 2/1/2024 6:42 PM, Andrew Lunn wrote: > On Thu, Feb 01, 2024 at 12:11:15PM +0530, Vineeth Karumanchi wrote: <...> >> The earlier implementation of WOL (magic-packet) was using DT property. >> We added one more packet type using DT property to be in-line with the >> earlier implementation. > > I can understand that. It also suggests we did a bad job reviewing > that patch, and should of rejected it. But it was added a long time > ago, and we were less strict back then. > >> >> However, I echo with you that this feature should be in driver (CAPS). >> We will re-work the implementation with the below flow: >> >> - Add MACB_CAPS_WOL capability to the supported platforms >> - Advertise supported WOL packet types based on the CAPS in ethtool. >> - Users can set packet type using ethtool. > > Yes, this sounds good. Maybe add to that, mark magic-packet > deprecated, and a comment that ethtool should be used instead. OK. We will implement above functionality and send V2. Thanks
diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml index bf8894a0257e..4bea177e85bc 100644 --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml @@ -144,6 +144,11 @@ patternProperties: description: Indicates that the hardware supports waking up via magic packet. + wol-arp-packet: + type: boolean + description: + Indicates that the hardware supports waking up via ARP packet. + unevaluatedProperties: false required:
"wol-arp-packet" property enables WOL with ARP packet. It is an extension to "magic-packet for WOL. Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com> --- 7c4a1d0cfdc1 net: macb: make magic-packet property generic which added magic-property support and wol-arp-packet addition is similar extension. --- Documentation/devicetree/bindings/net/cdns,macb.yaml | 5 +++++ 1 file changed, 5 insertions(+)