[iwl-net,v2] igc: Add support for receiving error frames

This patch enables the NIC to (optionally, via ethtool) receives
the errored packet frames as it was not provided to user before.

According to Software User Manual Chapter 8.9.1, once Bit(2) is set
in Receive Control Register (RCTL), bad packets will be received and
sent to host memory. Receive descriptor error field (RDESC.ERRORS)
shall have the corresponding bit to signal the driver that packets
is errored.

By turning on NETIF_F_RXALL as well, all broadcast packets will be
received and any flow control packets that aren't recognised will
be sent to the host.

How to test:
User can set to receive all frames using ethtool command.

Example command:
ethtool -K <interface> rx-all on

Previous output:

ethtool -K enp1s0 rx-all on
Actual changes:
rx-all: off [requested on]
Could not change any device features

New output:

ethtool -K enp1s0 rx-all on
ethtool -k enp1s0 | grep rx-all
rx-all: on

Fixes: 13b5b7fd6a4a ("igc: Add support for Tx/Rx rings")
Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Reviewed-by: Sasha Neftin <sasha.neftin@intel.com>
---
V1 -> V2: Fix typo in commit message
---
 drivers/net/ethernet/intel/igc/igc_main.c | 1 +
 1 file changed, 1 insertion(+)

On 9/12/2023 9:27 PM, Muhammad Husaini Zulkifli wrote:
> This patch enables the NIC to (optionally, via ethtool) receives
> the errored packet frames as it was not provided to user before.
> 
> According to Software User Manual Chapter 8.9.1, once Bit(2) is set
> in Receive Control Register (RCTL), bad packets will be received and
> sent to host memory. Receive descriptor error field (RDESC.ERRORS)
> shall have the corresponding bit to signal the driver that packets
> is errored.
> 
> By turning on NETIF_F_RXALL as well, all broadcast packets will be
> received and any flow control packets that aren't recognised will
> be sent to the host.
> 
> How to test:
> User can set to receive all frames using ethtool command.
> 
> Example command:
> ethtool -K <interface> rx-all on
> 
> Previous output:
> 
> ethtool -K enp1s0 rx-all on
> Actual changes:
> rx-all: off [requested on]
> Could not change any device features
> 
> New output:
> 
> ethtool -K enp1s0 rx-all on
> ethtool -k enp1s0 | grep rx-all
> rx-all: on
> 
> Fixes: 13b5b7fd6a4a ("igc: Add support for Tx/Rx rings")

What's the bug this is fixing? Seems like it's adding new functionality.

> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> Reviewed-by: Sasha Neftin <sasha.neftin@intel.com>
> ---
> V1 -> V2: Fix typo in commit message
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 98de34d0ce07..e3f4b3e95cd0 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6850,6 +6850,7 @@ static int igc_probe(struct pci_dev *pdev,
>   	netdev->hw_features |= NETIF_F_NTUPLE;
>   	netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_TX;
>   	netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
> +	netdev->hw_features |= NETIF_F_RXALL;

I questioned that this was all that was needed, however, looking at the 
driver, it looks like the the rest of the implementation is already 
present. It would be good to explain/add to the commit message.

>   	netdev->hw_features |= netdev->features;
>   
>   	netdev->features |= NETIF_F_HIGHDMA;

Dear Anthony,

> -----Original Message-----
> From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Sent: Friday, 15 September, 2023 4:17 AM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;
> intel-wired-lan@osuosl.org
> Cc: Neftin, Sasha <sasha.neftin@intel.com>; husainizulkifli@gmail.com;
> naamax.meir@linux.intel.com
> Subject: Re: [PATCH iwl-net v2] igc: Add support for receiving error frames
> 
> 
> 
> On 9/12/2023 9:27 PM, Muhammad Husaini Zulkifli wrote:
> > This patch enables the NIC to (optionally, via ethtool) receives the
> > errored packet frames as it was not provided to user before.
> >
> > According to Software User Manual Chapter 8.9.1, once Bit(2) is set in
> > Receive Control Register (RCTL), bad packets will be received and sent
> > to host memory. Receive descriptor error field (RDESC.ERRORS) shall
> > have the corresponding bit to signal the driver that packets is
> > errored.
> >
> > By turning on NETIF_F_RXALL as well, all broadcast packets will be
> > received and any flow control packets that aren't recognised will be
> > sent to the host.
> >
> > How to test:
> > User can set to receive all frames using ethtool command.
> >
> > Example command:
> > ethtool -K <interface> rx-all on
> >
> > Previous output:
> >
> > ethtool -K enp1s0 rx-all on
> > Actual changes:
> > rx-all: off [requested on]
> > Could not change any device features
> >
> > New output:
> >
> > ethtool -K enp1s0 rx-all on
> > ethtool -k enp1s0 | grep rx-all
> > rx-all: on
> >
> > Fixes: 13b5b7fd6a4a ("igc: Add support for Tx/Rx rings")
> 
> What's the bug this is fixing? Seems like it's adding new functionality.

The functionality is added with this ("igc: Add support for Tx/Rx rings").
But it seems like in-complete due to missing of " netdev->hw_features |= NETIF_F_RXALL;"
changes. We could not enable "rx-all" if this hw_features is not set.

> 
> > Signed-off-by: Muhammad Husaini Zulkifli
> > <muhammad.husaini.zulkifli@intel.com>
> > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com>
> > ---
> > V1 -> V2: Fix typo in commit message
> > ---
> >   drivers/net/ethernet/intel/igc/igc_main.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
> > b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 98de34d0ce07..e3f4b3e95cd0 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -6850,6 +6850,7 @@ static int igc_probe(struct pci_dev *pdev,
> >   	netdev->hw_features |= NETIF_F_NTUPLE;
> >   	netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_TX;
> >   	netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
> > +	netdev->hw_features |= NETIF_F_RXALL;
> 
> I questioned that this was all that was needed, however, looking at the driver,
> it looks like the the rest of the implementation is already present. It would be
> good to explain/add to the commit message.

I think it was mentioned in first paragraph in the commit message where I mentioned this was not
provided to user before.

Thanks,
Husaini

> 
> >   	netdev->hw_features |= netdev->features;
> >
> >   	netdev->features |= NETIF_F_HIGHDMA;