[PATCH net-next v2] net: thunderx: Make hfunc variable const type in nicvf_set_rxfh()

[Robert Richter]

>From struct ethtool_ops:

int (*set_rxfh)(struct net_device *, const u32 *indir,
const u8 *key, const u8 hfunc);

Change function arg of hfunc to const type.

V2: Fixed indentation.

Signed-off-by: Robert Richter <rrich...@cavium.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
index 2e74bbaa38e1..5ac474683c98 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
@@ -635,7 +635,7 @@ static int nicvf_get_rxfh(struct net_device *dev, u32 
*indir, u8 *hkey,
 }
 
 static int nicvf_set_rxfh(struct net_device *dev, const u32 *indir,
- const u8 *hkey, u8 hfunc)
+ const u8 *hkey, const u8 hfunc)
 {
struct nicvf *nic = netdev_priv(dev);
struct nicvf_rss_info *rss = >rss_info;
-- 
2.11.0

[PATCH net-next] net: thunderx: Make hfunc variable const type in nicvf_set_rxfh()

[Robert Richter]

>From struct ethtool_ops:

int (*set_rxfh)(struct net_device *, const u32 *indir,
const u8 *key, const u8 hfunc);

Change function arg of hfunc to const type.

Signed-off-by: Robert Richter <rrich...@cavium.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
index 2e74bbaa38e1..3a4761aa8efd 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
@@ -635,7 +635,7 @@ static int nicvf_get_rxfh(struct net_device *dev, u32 
*indir, u8 *hkey,
 }
 
 static int nicvf_set_rxfh(struct net_device *dev, const u32 *indir,
- const u8 *hkey, u8 hfunc)
+   const u8 *hkey, const u8 hfunc)
 {
struct nicvf *nic = netdev_priv(dev);
struct nicvf_rss_info *rss = >rss_info;
-- 
2.11.0

Re: [PATCH 2/2] net, thunderx: Use bool in structs where possible

[Robert Richter]

On 18.02.16 11:05:14, David Miller wrote:
> From: Robert Richter <rrich...@caviumnetworks.com>
> Date: Thu, 18 Feb 2016 13:39:09 +0100
> 
> > From: Robert Richter <rrich...@cavium.com>
> > 
> > Looks like the :1 notation was accidentally introduced (this still
> > uses 1 byte per flag). Using bool instead, which is the common use.
> > 
> > Signed-off-by: Robert Richter <rrich...@cavium.com>
> 
> Such cleanups are not appropriate for 'net'.  Only real bug fixes should
> be targetted that.
> 
> Respin these patches targetting the correct tree(s).

Do you mean posting the two as "trivial" patches?

Thanks,

-Robert

[PATCH 2/2] net, thunderx: Use bool in structs where possible

[Robert Richter]

From: Robert Richter <rrich...@cavium.com>

Looks like the :1 notation was accidentally introduced (this still
uses 1 byte per flag). Using bool instead, which is the common use.

Signed-off-by: Robert Richter <rrich...@cavium.com>
---
 drivers/net/ethernet/cavium/thunder/nic.h | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
b/drivers/net/ethernet/cavium/thunder/nic.h
index 688828865c48..fa5b1d2d8e23 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -262,9 +262,9 @@ struct nicvf {
struct pci_dev  *pdev;
u8  vf_id;
u8  node;
-   u8  tns_mode:1;
-   u8  sqs_mode:1;
-   u8  loopback_supported:1;
+   booltns_mode;
+   boolsqs_mode;
+   boolloopback_supported;
boolhw_tso;
u16 mtu;
struct queue_set*qs;
@@ -353,9 +353,9 @@ struct nic_cfg_msg {
u8msg;
u8vf_id;
u8node_id;
-   u8tns_mode:1;
-   u8sqs_mode:1;
-   u8loopback_supported:1;
+   bool  tns_mode;
+   bool  sqs_mode;
+   bool  loopback_supported;
u8mac_addr[ETH_ALEN];
 };
 
-- 
2.7.0.rc3

[PATCH 0/2] net: thunderx: Small cleanups

[Robert Richter]

From: Robert Richter <rrich...@cavium.com>

Sending two small cleanups of the driver fixing issues found during
code review.

Robert Richter (2):
  net: thunderx: Fix const type in nicvf_set_rxfh()
  net, thunderx: Use bool in structs where possible

 drivers/net/ethernet/cavium/thunder/nic.h   | 12 ++--
 drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.7.0.rc3

[PATCH 1/2] net: thunderx: Fix const type in nicvf_set_rxfh()

[Robert Richter]

From: Robert Richter <rrich...@cavium.com>

>From struct ethtool_ops:

int (*set_rxfh)(struct net_device *, const u32 *indir,
const u8 *key, const u8 hfunc);

Change function arg of hfunc to const type.

Signed-off-by: Robert Richter <rrich...@cavium.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
index a12b2e38cf61..fb79529ac8c7 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
@@ -581,7 +581,7 @@ static int nicvf_get_rxfh(struct net_device *dev, u32 
*indir, u8 *hkey,
 }
 
 static int nicvf_set_rxfh(struct net_device *dev, const u32 *indir,
- const u8 *hkey, u8 hfunc)
+   const u8 *hkey, const u8 hfunc)
 {
struct nicvf *nic = netdev_priv(dev);
struct nicvf_rss_info *rss = >rss_info;
-- 
2.7.0.rc3

Re: [PATCH 0/6] net: thunderx: Setting IRQ affinity hints and other optimizations

[Robert Richter]

On 11.02.16 09:33:41, David Miller wrote:
> From: Sunil Kovvuri 
> Date: Thu, 11 Feb 2016 18:56:48 +0530
> 
> > If time permits, can you please look at this patchset.
> 
> You were given feedback and I expect you to address that feedback
> and resubmit this series.
> 
> This is what the "Changed Requested" state in patchwork means.

Sunil, should I send a reworked version of my patch to you, or do you
just want to change it to dev_err() for me?

Thanks,

-Robert

Re: [PATCH 5/6] net: thunderx: bgx: Add log message when setting mac address

[Robert Richter]

On 08.02.16 16:30:37, Sergei Shtylyov wrote:
> >@@ -897,10 +898,13 @@ static int acpi_get_mac_address(struct acpi_device 
> >*adev, u8 *dst)
> > goto out;
> >
> > if (!is_valid_ether_addr(mac)) {
> >+dev_warn(dev, "MAC address invalid: %pM\n", mac);
> 
>dev_er(), maybe?

Since the driver may continue, my choice was a warning only.

-Robert

> 
> > ret = -EINVAL;
> > goto out;
> > }
> >
> >+dev_info(dev, "MAC address set to: %pM\n", mac);
> >+
> > memcpy(dst, mac, ETH_ALEN);
> >  out:
> > return ret;
> [...]

Re: [PATCH v2 0/2] net: thunder: Add ACPI support.

[Robert Richter]

On 11.08.15 13:04:55, David Daney wrote:
 In the future it might be better structured to try and get the OF
 node, and if that fails then try and use the ACPI method to obtain
 these values.
 
 Our current approach, as you can see in the patch, is the opposite.  If ACPI
 is being used, prefer that over the OF device tree.
 
 You seem to be recommending precedence for OF.  It should be consistent
 across all drivers/sub-systems, so do you really think that OF before ACPI
 is the way to go?

If ACPI is enabled then no OF function may be called at all.

With !ACPI or acpi=no kernel parameter, then acpi_disabled is set and
no ACPI function should be called. It always falls back to and only
uses OF/devicetree in this case.

So there is now way to try devicetree first and then use acpi or vice
versa. There is no mixup using acpi or devicetree with the same boot,
either one or the other.

-Robert
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.

[Robert Richter]

On 07.08.15 10:09:04, Tomasz Nowicki wrote:
 On 07.08.2015 02:33, David Daney wrote:

...

 +#else
 +
 +static int bgx_init_acpi_phy(struct bgx *bgx)
 +{
 +return -ENODEV;
 +}
 +
 +#endif /* CONFIG_ACPI */
 +
   #if IS_ENABLED(CONFIG_OF_MDIO)
 
   static int bgx_init_of_phy(struct bgx *bgx)
 @@ -882,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
 
   static int bgx_init_phy(struct bgx *bgx)
   {
 -return bgx_init_of_phy(bgx);
 +int err = bgx_init_of_phy(bgx);
 +
 +if (err != -ENODEV)
 +return err;
 +
 +return bgx_init_acpi_phy(bgx);
   }
 
 
 If kernel can work with DT and ACPI (both compiled in), it should take only
 one path instead of probing DT and ACPI sequentially. How about:
 
 @@ -902,10 +925,9 @@ static int bgx_probe(struct pci_dev *pdev, const struct
 pci_device_id *ent)
   bgx_vnic[bgx-bgx_id] = bgx;
   bgx_get_qlm_mode(bgx);
 
 - snprintf(bgx_sel, 5, bgx%d, bgx-bgx_id);
 - np = of_find_node_by_name(NULL, bgx_sel);
 - if (np)
 - bgx_init_of(bgx, np);
 + err = acpi_disabled ? bgx_init_of_phy(bgx) : bgx_init_acpi_phy(bgx);
 + if (err)
 + goto err_enable;
 
   bgx_init_hw(bgx);

I would not pollute bgx_probe() with acpi and dt specifics, and instead
keep bgx_init_phy(). The typical design pattern for this is:

static int bgx_init_phy(struct bgx *bgx)
{
#ifdef CONFIG_ACPI
if (!acpi_disabled)
return bgx_init_acpi_phy(bgx);
#endif
return bgx_init_of_phy(bgx);
}

This adds acpi runtime detection (acpi=no), does not call dt code in
case of acpi, and saves the #else for bgx_init_acpi_phy().

-Robert
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] net, thunder, bgx: Add support for ACPI binding.

[Robert Richter]

On 07.08.15 12:52:41, Tomasz Nowicki wrote:
 On 07.08.2015 12:43, Robert Richter wrote:
 On 07.08.15 10:09:04, Tomasz Nowicki wrote:
 On 07.08.2015 02:33, David Daney wrote:
 
 ...
 
 +#else
 +
 +static int bgx_init_acpi_phy(struct bgx *bgx)
 +{
 +  return -ENODEV;
 +}
 +
 +#endif /* CONFIG_ACPI */
 +
   #if IS_ENABLED(CONFIG_OF_MDIO)
 
   static int bgx_init_of_phy(struct bgx *bgx)
 @@ -882,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
 
   static int bgx_init_phy(struct bgx *bgx)
   {
 -  return bgx_init_of_phy(bgx);
 +  int err = bgx_init_of_phy(bgx);
 +
 +  if (err != -ENODEV)
 +  return err;
 +
 +  return bgx_init_acpi_phy(bgx);
   }
 
 
 If kernel can work with DT and ACPI (both compiled in), it should take only
 one path instead of probing DT and ACPI sequentially. How about:
 
 @@ -902,10 +925,9 @@ static int bgx_probe(struct pci_dev *pdev, const struct
 pci_device_id *ent)
 bgx_vnic[bgx-bgx_id] = bgx;
 bgx_get_qlm_mode(bgx);
 
 -   snprintf(bgx_sel, 5, bgx%d, bgx-bgx_id);
 -   np = of_find_node_by_name(NULL, bgx_sel);
 -   if (np)
 -   bgx_init_of(bgx, np);
 +   err = acpi_disabled ? bgx_init_of_phy(bgx) : bgx_init_acpi_phy(bgx);
 +   if (err)
 +   goto err_enable;
 
 bgx_init_hw(bgx);
 
 I would not pollute bgx_probe() with acpi and dt specifics, and instead
 keep bgx_init_phy(). The typical design pattern for this is:
 
 static int bgx_init_phy(struct bgx *bgx)
 {
 #ifdef CONFIG_ACPI
  if (!acpi_disabled)
  return bgx_init_acpi_phy(bgx);
 #endif
  return bgx_init_of_phy(bgx);
 }
 
 This adds acpi runtime detection (acpi=no), does not call dt code in
 case of acpi, and saves the #else for bgx_init_acpi_phy().
 
 
 I am fine with keeping it in bgx_init_phy(), however we can drop there
 #ifdefs since both of bgx_init_{acpi,of}_phy calls have empty stub for !ACPI
 and !OF case. Like that:
 
 static int bgx_init_phy(struct bgx *bgx)
 {
 
 if (!acpi_disabled)
 return bgx_init_acpi_phy(bgx);
   else
   return bgx_init_of_phy(bgx);
 }

As said, keeping it in #ifdefs makes the empty stub function for !acpi
obsolete, which makes the code smaller and better readable. This style
is common practice in the kernel. Apart from that, the 'else' should
be dropped as it is useless.

-Robert
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html