Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[David Miller]

From: Madalin-Cristian Bucur 
Date: Wed, 9 Nov 2016 17:16:12 +

>> From: Madalin-Cristian Bucur
>> Sent: Monday, November 07, 2016 5:43 PM
>> 
>> > From: David Miller [mailto:da...@davemloft.net]
>> > Sent: Thursday, November 03, 2016 9:58 PM
>> >
>> > From: Madalin Bucur 
>> > Date: Wed, 2 Nov 2016 22:17:26 +0200
>> >
>> > > This introduces the Freescale Data Path Acceleration Architecture
> 
> 
>> > > +int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
>> >  ...
>> > > +cpustats = (u64 *)_priv->stats;
>> > > +
>> > > +for (j = 0; j < numstats; j++)
>> > > +netstats[j] += cpustats[j];
>> >
>> > This is a memcpy() on well-typed datastructures which requires no
>> > casting or special handling whatsoever, so use memcpy instead of
>> > needlessly open coding the operation.
>> 
>> Will fix.
> 
> Took a second look at this, it's not copying but adding the percpu
> statistics into consolidated results.

Ok, then it looks fine, thanks for clarifying.

Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[David Miller]

From: Madalin-Cristian Bucur 
Date: Wed, 9 Nov 2016 17:16:12 +

>> From: Madalin-Cristian Bucur
>> Sent: Monday, November 07, 2016 5:43 PM
>> 
>> > From: David Miller [mailto:da...@davemloft.net]
>> > Sent: Thursday, November 03, 2016 9:58 PM
>> >
>> > From: Madalin Bucur 
>> > Date: Wed, 2 Nov 2016 22:17:26 +0200
>> >
>> > > This introduces the Freescale Data Path Acceleration Architecture
> 
> 
>> > > +int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
>> >  ...
>> > > +cpustats = (u64 *)_priv->stats;
>> > > +
>> > > +for (j = 0; j < numstats; j++)
>> > > +netstats[j] += cpustats[j];
>> >
>> > This is a memcpy() on well-typed datastructures which requires no
>> > casting or special handling whatsoever, so use memcpy instead of
>> > needlessly open coding the operation.
>> 
>> Will fix.
> 
> Took a second look at this, it's not copying but adding the percpu
> statistics into consolidated results.

Ok, then it looks fine, thanks for clarifying.

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> From: Madalin-Cristian Bucur
> Sent: Monday, November 07, 2016 5:43 PM
> 
> > From: David Miller [mailto:da...@davemloft.net]
> > Sent: Thursday, November 03, 2016 9:58 PM
> >
> > From: Madalin Bucur 
> > Date: Wed, 2 Nov 2016 22:17:26 +0200
> >
> > > This introduces the Freescale Data Path Acceleration Architecture


> > > + int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
> >  ...
> > > + cpustats = (u64 *)_priv->stats;
> > > +
> > > + for (j = 0; j < numstats; j++)
> > > + netstats[j] += cpustats[j];
> >
> > This is a memcpy() on well-typed datastructures which requires no
> > casting or special handling whatsoever, so use memcpy instead of
> > needlessly open coding the operation.
> 
> Will fix.

Took a second look at this, it's not copying but adding the percpu
statistics into consolidated results.

Madalin

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> From: Madalin-Cristian Bucur
> Sent: Monday, November 07, 2016 5:43 PM
> 
> > From: David Miller [mailto:da...@davemloft.net]
> > Sent: Thursday, November 03, 2016 9:58 PM
> >
> > From: Madalin Bucur 
> > Date: Wed, 2 Nov 2016 22:17:26 +0200
> >
> > > This introduces the Freescale Data Path Acceleration Architecture


> > > + int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
> >  ...
> > > + cpustats = (u64 *)_priv->stats;
> > > +
> > > + for (j = 0; j < numstats; j++)
> > > + netstats[j] += cpustats[j];
> >
> > This is a memcpy() on well-typed datastructures which requires no
> > casting or special handling whatsoever, so use memcpy instead of
> > needlessly open coding the operation.
> 
> Will fix.

Took a second look at this, it's not copying but adding the percpu
statistics into consolidated results.

Madalin

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Monday, November 07, 2016 5:55 PM
> 
> From: Madalin-Cristian Bucur 
> Date: Mon, 7 Nov 2016 15:43:26 +
> 
> >> From: David Miller [mailto:da...@davemloft.net]
> >> Sent: Thursday, November 03, 2016 9:58 PM
> >>
> >> Why?  By clearing this, you disallow an important fundamental way to do
> >> performane testing, via pktgen.
> >
> > The Tx path in DPAA requires one to insert a back-pointer to the skb
> into
> > the Tx buffer. On the Tx confirmation path the back-pointer in the
> buffer
> > is used to release the skb. If Tx buffer is shared we'd alter the back-
> pointer
> > and leak/double free skbs. See also
> 
> Then have your software state store an array of SKB pointers, one for each
> TX ring entry, just like every other driver does.

There is no Tx ring in DPAA. Frames are send out on QMan HW queues towards
the FMan for Tx and then received back on Tx confirmation queues for cleanup.
Array traversal would for sure cost more than using the back-pointer. Also,
we can now process confirmations on a different core than the one doing Tx,
we'd have to keep the arrays percpu and force the Tx conf on the same core.
Or add locks.

Madalin

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Monday, November 07, 2016 5:55 PM
> 
> From: Madalin-Cristian Bucur 
> Date: Mon, 7 Nov 2016 15:43:26 +
> 
> >> From: David Miller [mailto:da...@davemloft.net]
> >> Sent: Thursday, November 03, 2016 9:58 PM
> >>
> >> Why?  By clearing this, you disallow an important fundamental way to do
> >> performane testing, via pktgen.
> >
> > The Tx path in DPAA requires one to insert a back-pointer to the skb
> into
> > the Tx buffer. On the Tx confirmation path the back-pointer in the
> buffer
> > is used to release the skb. If Tx buffer is shared we'd alter the back-
> pointer
> > and leak/double free skbs. See also
> 
> Then have your software state store an array of SKB pointers, one for each
> TX ring entry, just like every other driver does.

There is no Tx ring in DPAA. Frames are send out on QMan HW queues towards
the FMan for Tx and then received back on Tx confirmation queues for cleanup.
Array traversal would for sure cost more than using the back-pointer. Also,
we can now process confirmations on a different core than the one doing Tx,
we'd have to keep the arrays percpu and force the Tx conf on the same core.
Or add locks.

Madalin

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Monday, November 07, 2016 6:40 PM
> 
> From: Madalin-Cristian Bucur 
> Date: Mon, 7 Nov 2016 16:32:16 +
> 
> >> From: David Miller [mailto:da...@davemloft.net]
> >> Sent: Monday, November 07, 2016 5:55 PM
> >>
> >> From: Madalin-Cristian Bucur 
> >> Date: Mon, 7 Nov 2016 15:43:26 +
> >>
> >> >> From: David Miller [mailto:da...@davemloft.net]
> >> >> Sent: Thursday, November 03, 2016 9:58 PM
> >> >>
> >> >> Why?  By clearing this, you disallow an important fundamental way to
> >> >> do performane testing, via pktgen.
> >> >
> >> > The Tx path in DPAA requires one to insert a back-pointer to the skb
> >> > into
> >> > the Tx buffer. On the Tx confirmation path the back-pointer in the
> >> > buffer
> >> > is used to release the skb. If Tx buffer is shared we'd alter the
> >> > back-pointer
> >> > and leak/double free skbs. See also
> >>
> >> Then have your software state store an array of SKB pointers, one for
> each
> >> TX ring entry, just like every other driver does.
> >
> > There is no Tx ring in DPAA. Frames are send out on QMan HW queues
> > towards the FMan for Tx and then received back on Tx confirmation queues
> > for cleanup.
> > Array traversal would for sure cost more than using the back-pointer.
> > Also, we can now process confirmations on a different core than the one
> > doing Tx,
> > we'd have to keep the arrays percpu and force the Tx conf on the same
> > core. Or add locks.
> 
> Report back an integer index, like every scsi driver out there which
> completes tagged queued block I/O operations asynchronously.  You can
> associate the array with a specific TX confirmation queue.

>From HW? It only gives you back the buffer start address (plus length, etc).
"buff_2_skb()" needs to be solved in SW, expensively using array (lists? As
the number of frames in flight can be large/variable) or cheaply with the back
pointer. The back-pointer approach has its tradeoffs: no shared skbs, imposed
non-zero needed_headroom.

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Monday, November 07, 2016 6:40 PM
> 
> From: Madalin-Cristian Bucur 
> Date: Mon, 7 Nov 2016 16:32:16 +
> 
> >> From: David Miller [mailto:da...@davemloft.net]
> >> Sent: Monday, November 07, 2016 5:55 PM
> >>
> >> From: Madalin-Cristian Bucur 
> >> Date: Mon, 7 Nov 2016 15:43:26 +
> >>
> >> >> From: David Miller [mailto:da...@davemloft.net]
> >> >> Sent: Thursday, November 03, 2016 9:58 PM
> >> >>
> >> >> Why?  By clearing this, you disallow an important fundamental way to
> >> >> do performane testing, via pktgen.
> >> >
> >> > The Tx path in DPAA requires one to insert a back-pointer to the skb
> >> > into
> >> > the Tx buffer. On the Tx confirmation path the back-pointer in the
> >> > buffer
> >> > is used to release the skb. If Tx buffer is shared we'd alter the
> >> > back-pointer
> >> > and leak/double free skbs. See also
> >>
> >> Then have your software state store an array of SKB pointers, one for
> each
> >> TX ring entry, just like every other driver does.
> >
> > There is no Tx ring in DPAA. Frames are send out on QMan HW queues
> > towards the FMan for Tx and then received back on Tx confirmation queues
> > for cleanup.
> > Array traversal would for sure cost more than using the back-pointer.
> > Also, we can now process confirmations on a different core than the one
> > doing Tx,
> > we'd have to keep the arrays percpu and force the Tx conf on the same
> > core. Or add locks.
> 
> Report back an integer index, like every scsi driver out there which
> completes tagged queued block I/O operations asynchronously.  You can
> associate the array with a specific TX confirmation queue.

>From HW? It only gives you back the buffer start address (plus length, etc).
"buff_2_skb()" needs to be solved in SW, expensively using array (lists? As
the number of frames in flight can be large/variable) or cheaply with the back
pointer. The back-pointer approach has its tradeoffs: no shared skbs, imposed
non-zero needed_headroom.

Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[David Miller]

From: Madalin-Cristian Bucur 
Date: Mon, 7 Nov 2016 16:32:16 +

>> -Original Message-
>> From: David Miller [mailto:da...@davemloft.net]
>> Sent: Monday, November 07, 2016 5:55 PM
>> 
>> From: Madalin-Cristian Bucur 
>> Date: Mon, 7 Nov 2016 15:43:26 +
>> 
>> >> From: David Miller [mailto:da...@davemloft.net]
>> >> Sent: Thursday, November 03, 2016 9:58 PM
>> >>
>> >> Why?  By clearing this, you disallow an important fundamental way to do
>> >> performane testing, via pktgen.
>> >
>> > The Tx path in DPAA requires one to insert a back-pointer to the skb
>> into
>> > the Tx buffer. On the Tx confirmation path the back-pointer in the
>> buffer
>> > is used to release the skb. If Tx buffer is shared we'd alter the back-
>> pointer
>> > and leak/double free skbs. See also
>> 
>> Then have your software state store an array of SKB pointers, one for each
>> TX ring entry, just like every other driver does.
> 
> There is no Tx ring in DPAA. Frames are send out on QMan HW queues towards
> the FMan for Tx and then received back on Tx confirmation queues for cleanup.
> Array traversal would for sure cost more than using the back-pointer. Also,
> we can now process confirmations on a different core than the one doing Tx,
> we'd have to keep the arrays percpu and force the Tx conf on the same core.
> Or add locks.

Report back an integer index, like every scsi driver out there which
completes tagged queued block I/O operations asynchronously.  You can
associate the array with a specific TX confirmation queue.

Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[David Miller]

From: Madalin-Cristian Bucur 
Date: Mon, 7 Nov 2016 16:32:16 +

>> -Original Message-
>> From: David Miller [mailto:da...@davemloft.net]
>> Sent: Monday, November 07, 2016 5:55 PM
>> 
>> From: Madalin-Cristian Bucur 
>> Date: Mon, 7 Nov 2016 15:43:26 +
>> 
>> >> From: David Miller [mailto:da...@davemloft.net]
>> >> Sent: Thursday, November 03, 2016 9:58 PM
>> >>
>> >> Why?  By clearing this, you disallow an important fundamental way to do
>> >> performane testing, via pktgen.
>> >
>> > The Tx path in DPAA requires one to insert a back-pointer to the skb
>> into
>> > the Tx buffer. On the Tx confirmation path the back-pointer in the
>> buffer
>> > is used to release the skb. If Tx buffer is shared we'd alter the back-
>> pointer
>> > and leak/double free skbs. See also
>> 
>> Then have your software state store an array of SKB pointers, one for each
>> TX ring entry, just like every other driver does.
> 
> There is no Tx ring in DPAA. Frames are send out on QMan HW queues towards
> the FMan for Tx and then received back on Tx confirmation queues for cleanup.
> Array traversal would for sure cost more than using the back-pointer. Also,
> we can now process confirmations on a different core than the one doing Tx,
> we'd have to keep the arrays percpu and force the Tx conf on the same core.
> Or add locks.

Report back an integer index, like every scsi driver out there which
completes tagged queued block I/O operations asynchronously.  You can
associate the array with a specific TX confirmation queue.

Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Joakim Tjernlund]

On Wed, 2016-11-02 at 22:17 +0200, Madalin Bucur wrote:
> This introduces the Freescale Data Path Acceleration Architecture
> (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
> BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
> the Freescale DPAA QorIQ platforms.

Nice to see DPAA support soon entering the kernel(not a day too early:)
I would like to see BQL supported from day one though, if possible.

 Regards
          Joakim Tjernlund

Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Joakim Tjernlund]

On Wed, 2016-11-02 at 22:17 +0200, Madalin Bucur wrote:
> This introduces the Freescale Data Path Acceleration Architecture
> (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
> BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
> the Freescale DPAA QorIQ platforms.

Nice to see DPAA support soon entering the kernel(not a day too early:)
I would like to see BQL supported from day one though, if possible.

 Regards
          Joakim Tjernlund

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Thursday, November 03, 2016 9:58 PM
> 
> From: Madalin Bucur 
> Date: Wed, 2 Nov 2016 22:17:26 +0200
> 
> > This introduces the Freescale Data Path Acceleration Architecture
> > +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> > +{
> > +   u8 i;
> > +   size_t res = DPAA_BP_RAW_SIZE / 2;
> 
> Always order local variable declarations from longest to shortest line,
> also know as Reverse Christmas Tree Format.
> 
> Please audit your entire submission for this problem, it occurs
> everywhere.

Thank you, I'll resolve this.

> > +   /* we do not want shared skbs on TX */
> > +   net_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> 
> Why?  By clearing this, you disallow an important fundamental way to do
> performane testing, via pktgen.

The Tx path in DPAA requires one to insert a back-pointer to the skb into
the Tx buffer. On the Tx confirmation path the back-pointer in the buffer
is used to release the skb. If Tx buffer is shared we'd alter the back-pointer
and leak/double free skbs. See also 

static int dpaa_start_xmit(struct sk_buff *skb, struct net_device 
*net_dev)
{
...
  if (!nonlinear) {
/* We're going to store the skb backpointer at the 
beginning
 * of the data buffer, so we need a privately owned skb
   *
 * We've made sure skb is not shared in dev->priv_flags,
 * we need to verify the skb head is not cloned
   */
if (skb_cow_head(skb, priv->tx_headroom))
goto enomem;

  WARN_ON(skb_is_nonlinear(skb));
}
...

> > +   int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
>  ...
> > +   cpustats = (u64 *)_priv->stats;
> > +
> > +   for (j = 0; j < numstats; j++)
> > +   netstats[j] += cpustats[j];
> 
> This is a memcpy() on well-typed datastructures which requires no
> casting or special handling whatsoever, so use memcpy instead of
> needlessly open coding the operation.

Will fix.

> > +static int dpaa_change_mtu(struct net_device *net_dev, int new_mtu)
> > +{
> > +   const int max_mtu = dpaa_get_max_mtu();
> > +
> > +   /* Make sure we don't exceed the Ethernet controller's MAXFRM */
> > +   if (new_mtu < 68 || new_mtu > max_mtu) {
> > +   netdev_err(net_dev, "Invalid L3 mtu %d (must be between %d and
> %d).\n",
> > +  new_mtu, 68, max_mtu);
> > +   return -EINVAL;
> > +   }
> > +   net_dev->mtu = new_mtu;
> > +
> > +   return 0;
> > +}
> 
> MTU restrictions are handled in the net-next tree via net_dev->min_mtu and
> net_dev->max_mtu.  Use that and do not define this NDO operation as you do
> not need it.

OK
 
> > +static int dpaa_set_features(struct net_device *dev, netdev_features_t
> features)
> > +{
> > +   /* Not much to do here for now */
> > +   dev->features = features;
> > +   return 0;
> > +}
> 
> Do not define unnecessary NDO operations, let the defaults do their job.
> 
> > +static netdev_features_t dpaa_fix_features(struct net_device *dev,
> > +  netdev_features_t features)
> > +{
> > +   netdev_features_t unsupported_features = 0;
> > +
> > +   /* In theory we should never be requested to enable features that
> > +* we didn't set in netdev->features and netdev->hw_features at
> probe
> > +* time, but double check just to be on the safe side.
> > +*/
> > +   unsupported_features |= NETIF_F_RXCSUM;
> > +
> > +   features &= ~unsupported_features;
> > +
> > +   return features;
> > +}
> 
> Unless you can show that your need this, do not "guess" by implement this
> NDO operation.  You don't need it.

Will remove it.
 
> > +#ifdef CONFIG_FSL_DPAA_ETH_FRIENDLY_IF_NAME
> > +static int dpaa_mac_hw_index_get(struct platform_device *pdev)
> > +{
> > +   struct device *dpaa_dev;
> > +   struct dpaa_eth_data *eth_data;
> > +
> > +   dpaa_dev = >dev;
> > +   eth_data = dpaa_dev->platform_data;
> > +
> > +   return eth_data->mac_hw_id;
> > +}
> > +
> > +static int dpaa_mac_fman_index_get(struct platform_device *pdev)
> > +{
> > +   struct device *dpaa_dev;
> > +   struct dpaa_eth_data *eth_data;
> > +
> > +   dpaa_dev = >dev;
> > +   eth_data = dpaa_dev->platform_data;
> > +
> > +   return eth_data->fman_hw_id;
> > +}
> > +#endif
> 
> Do not play network device naming games like this, use the standard name
> assignment done by the kernel and have userspace entities do geographic or
> device type specific naming.
> 
> I want to see this code completely removed.

I'll remove the option, udev rules like these can achieve the same effect:

SUBSYSTEM=="net", DRIVERS=="fsl_dpa*", ATTR{device_addr}=="ffe4e", 
NAME="fm1-mac1"
SUBSYSTEM=="net", DRIVERS=="fsl_dpa*", ATTR{device_addr}=="ffe4e2000", 
NAME="fm1-mac2"
 
> > +static int 

RE: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[Madalin-Cristian Bucur]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Thursday, November 03, 2016 9:58 PM
> 
> From: Madalin Bucur 
> Date: Wed, 2 Nov 2016 22:17:26 +0200
> 
> > This introduces the Freescale Data Path Acceleration Architecture
> > +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> > +{
> > +   u8 i;
> > +   size_t res = DPAA_BP_RAW_SIZE / 2;
> 
> Always order local variable declarations from longest to shortest line,
> also know as Reverse Christmas Tree Format.
> 
> Please audit your entire submission for this problem, it occurs
> everywhere.

Thank you, I'll resolve this.

> > +   /* we do not want shared skbs on TX */
> > +   net_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> 
> Why?  By clearing this, you disallow an important fundamental way to do
> performane testing, via pktgen.

The Tx path in DPAA requires one to insert a back-pointer to the skb into
the Tx buffer. On the Tx confirmation path the back-pointer in the buffer
is used to release the skb. If Tx buffer is shared we'd alter the back-pointer
and leak/double free skbs. See also 

static int dpaa_start_xmit(struct sk_buff *skb, struct net_device 
*net_dev)
{
...
  if (!nonlinear) {
/* We're going to store the skb backpointer at the 
beginning
 * of the data buffer, so we need a privately owned skb
   *
 * We've made sure skb is not shared in dev->priv_flags,
 * we need to verify the skb head is not cloned
   */
if (skb_cow_head(skb, priv->tx_headroom))
goto enomem;

  WARN_ON(skb_is_nonlinear(skb));
}
...

> > +   int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
>  ...
> > +   cpustats = (u64 *)_priv->stats;
> > +
> > +   for (j = 0; j < numstats; j++)
> > +   netstats[j] += cpustats[j];
> 
> This is a memcpy() on well-typed datastructures which requires no
> casting or special handling whatsoever, so use memcpy instead of
> needlessly open coding the operation.

Will fix.

> > +static int dpaa_change_mtu(struct net_device *net_dev, int new_mtu)
> > +{
> > +   const int max_mtu = dpaa_get_max_mtu();
> > +
> > +   /* Make sure we don't exceed the Ethernet controller's MAXFRM */
> > +   if (new_mtu < 68 || new_mtu > max_mtu) {
> > +   netdev_err(net_dev, "Invalid L3 mtu %d (must be between %d and
> %d).\n",
> > +  new_mtu, 68, max_mtu);
> > +   return -EINVAL;
> > +   }
> > +   net_dev->mtu = new_mtu;
> > +
> > +   return 0;
> > +}
> 
> MTU restrictions are handled in the net-next tree via net_dev->min_mtu and
> net_dev->max_mtu.  Use that and do not define this NDO operation as you do
> not need it.

OK
 
> > +static int dpaa_set_features(struct net_device *dev, netdev_features_t
> features)
> > +{
> > +   /* Not much to do here for now */
> > +   dev->features = features;
> > +   return 0;
> > +}
> 
> Do not define unnecessary NDO operations, let the defaults do their job.
> 
> > +static netdev_features_t dpaa_fix_features(struct net_device *dev,
> > +  netdev_features_t features)
> > +{
> > +   netdev_features_t unsupported_features = 0;
> > +
> > +   /* In theory we should never be requested to enable features that
> > +* we didn't set in netdev->features and netdev->hw_features at
> probe
> > +* time, but double check just to be on the safe side.
> > +*/
> > +   unsupported_features |= NETIF_F_RXCSUM;
> > +
> > +   features &= ~unsupported_features;
> > +
> > +   return features;
> > +}
> 
> Unless you can show that your need this, do not "guess" by implement this
> NDO operation.  You don't need it.

Will remove it.
 
> > +#ifdef CONFIG_FSL_DPAA_ETH_FRIENDLY_IF_NAME
> > +static int dpaa_mac_hw_index_get(struct platform_device *pdev)
> > +{
> > +   struct device *dpaa_dev;
> > +   struct dpaa_eth_data *eth_data;
> > +
> > +   dpaa_dev = >dev;
> > +   eth_data = dpaa_dev->platform_data;
> > +
> > +   return eth_data->mac_hw_id;
> > +}
> > +
> > +static int dpaa_mac_fman_index_get(struct platform_device *pdev)
> > +{
> > +   struct device *dpaa_dev;
> > +   struct dpaa_eth_data *eth_data;
> > +
> > +   dpaa_dev = >dev;
> > +   eth_data = dpaa_dev->platform_data;
> > +
> > +   return eth_data->fman_hw_id;
> > +}
> > +#endif
> 
> Do not play network device naming games like this, use the standard name
> assignment done by the kernel and have userspace entities do geographic or
> device type specific naming.
> 
> I want to see this code completely removed.

I'll remove the option, udev rules like these can achieve the same effect:

SUBSYSTEM=="net", DRIVERS=="fsl_dpa*", ATTR{device_addr}=="ffe4e", 
NAME="fm1-mac1"
SUBSYSTEM=="net", DRIVERS=="fsl_dpa*", ATTR{device_addr}=="ffe4e2000", 
NAME="fm1-mac2"
 
> > +static int dpaa_set_mac_address(struct net_device 

Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[David Miller]

From: Madalin-Cristian Bucur 
Date: Mon, 7 Nov 2016 15:43:26 +

>> From: David Miller [mailto:da...@davemloft.net]
>> Sent: Thursday, November 03, 2016 9:58 PM
>> 
>> Why?  By clearing this, you disallow an important fundamental way to do
>> performane testing, via pktgen.
> 
> The Tx path in DPAA requires one to insert a back-pointer to the skb into
> the Tx buffer. On the Tx confirmation path the back-pointer in the buffer
> is used to release the skb. If Tx buffer is shared we'd alter the back-pointer
> and leak/double free skbs. See also 

Then have your software state store an array of SKB pointers, one for each
TX ring entry, just like every other driver does.

Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[David Miller]

From: Madalin-Cristian Bucur 
Date: Mon, 7 Nov 2016 15:43:26 +

>> From: David Miller [mailto:da...@davemloft.net]
>> Sent: Thursday, November 03, 2016 9:58 PM
>> 
>> Why?  By clearing this, you disallow an important fundamental way to do
>> performane testing, via pktgen.
> 
> The Tx path in DPAA requires one to insert a back-pointer to the skb into
> the Tx buffer. On the Tx confirmation path the back-pointer in the buffer
> is used to release the skb. If Tx buffer is shared we'd alter the back-pointer
> and leak/double free skbs. See also 

Then have your software state store an array of SKB pointers, one for each
TX ring entry, just like every other driver does.

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Michael Ellerman]

Joe Perches  writes:
> On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
>> From: Madalin Bucur 
>> Date: Wed, 2 Nov 2016 22:17:26 +0200
>> 
>> > This introduces the Freescale Data Path Acceleration Architecture
>> > +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
>> > +{
>> > + u8 i;
>> > + size_t res = DPAA_BP_RAW_SIZE / 2;
>> 
>> Always order local variable declarations from longest to shortest line,
>> also know as Reverse Christmas Tree Format.
>
> I think this declaration sorting order is misguided but
> here's a possible change to checkpatch adding a test for it
> that does this test just for net/ and drivers/net/

And arch/powerpc too please.

cheers

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Michael Ellerman]

Joe Perches  writes:
> On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
>> From: Madalin Bucur 
>> Date: Wed, 2 Nov 2016 22:17:26 +0200
>> 
>> > This introduces the Freescale Data Path Acceleration Architecture
>> > +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
>> > +{
>> > + u8 i;
>> > + size_t res = DPAA_BP_RAW_SIZE / 2;
>> 
>> Always order local variable declarations from longest to shortest line,
>> also know as Reverse Christmas Tree Format.
>
> I think this declaration sorting order is misguided but
> here's a possible change to checkpatch adding a test for it
> that does this test just for net/ and drivers/net/

And arch/powerpc too please.

cheers

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[David VomLehn]

On Fri, Nov 04, 2016 at 10:05:15AM -0700, Randy Dunlap wrote:
> On 11/03/16 23:53, Joe Perches wrote:
> > On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
> >> From: Madalin Bucur 
> >> Date: Wed, 2 Nov 2016 22:17:26 +0200
> >>
> >>> This introduces the Freescale Data Path Acceleration Architecture
> >>> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> >>> +{
> >>> + u8 i;
> >>> + size_t res = DPAA_BP_RAW_SIZE / 2;
> >>
> >> Always order local variable declarations from longest to shortest line,
> >> also know as Reverse Christmas Tree Format.
> > 
> > I think this declaration sorting order is misguided but
> > here's a possible change to checkpatch adding a test for it
> > that does this test just for net/ and drivers/net/
> 
> I agree with the misguided part.
> That's not actually in CodingStyle AFAICT. Where did this come from?
> 
> 
> thanks.
> -- 
> ~Randy

This puzzles me. The CodingStyle gives some pretty reasonable rationales
for coding style over above the "it's easier to read if it all looks the
same". I can see rationales for other approaches (and I am not proposing
any of these):

alphabetic orderEasier to search for declarations
complex to simple   As in, structs and unions, pointers to simple
data (int, char), simple data. It seems like I
can deduce the simple types from usage, but more
complex I need to know things like the
particular structure.
group by usage  Mirror the ontological locality in the code

Do we have a basis for thinking this is easier or more consistent than
any other approach?
-- 
David VL

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[David VomLehn]

On Fri, Nov 04, 2016 at 10:05:15AM -0700, Randy Dunlap wrote:
> On 11/03/16 23:53, Joe Perches wrote:
> > On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
> >> From: Madalin Bucur 
> >> Date: Wed, 2 Nov 2016 22:17:26 +0200
> >>
> >>> This introduces the Freescale Data Path Acceleration Architecture
> >>> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> >>> +{
> >>> + u8 i;
> >>> + size_t res = DPAA_BP_RAW_SIZE / 2;
> >>
> >> Always order local variable declarations from longest to shortest line,
> >> also know as Reverse Christmas Tree Format.
> > 
> > I think this declaration sorting order is misguided but
> > here's a possible change to checkpatch adding a test for it
> > that does this test just for net/ and drivers/net/
> 
> I agree with the misguided part.
> That's not actually in CodingStyle AFAICT. Where did this come from?
> 
> 
> thanks.
> -- 
> ~Randy

This puzzles me. The CodingStyle gives some pretty reasonable rationales
for coding style over above the "it's easier to read if it all looks the
same". I can see rationales for other approaches (and I am not proposing
any of these):

alphabetic orderEasier to search for declarations
complex to simple   As in, structs and unions, pointers to simple
data (int, char), simple data. It seems like I
can deduce the simple types from usage, but more
complex I need to know things like the
particular structure.
group by usage  Mirror the ontological locality in the code

Do we have a basis for thinking this is easier or more consistent than
any other approach?
-- 
David VL

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Randy Dunlap]

On 11/03/16 23:53, Joe Perches wrote:
> On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
>> From: Madalin Bucur 
>> Date: Wed, 2 Nov 2016 22:17:26 +0200
>>
>>> This introduces the Freescale Data Path Acceleration Architecture
>>> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
>>> +{
>>> + u8 i;
>>> + size_t res = DPAA_BP_RAW_SIZE / 2;
>>
>> Always order local variable declarations from longest to shortest line,
>> also know as Reverse Christmas Tree Format.
> 
> I think this declaration sorting order is misguided but
> here's a possible change to checkpatch adding a test for it
> that does this test just for net/ and drivers/net/

I agree with the misguided part.
That's not actually in CodingStyle AFAICT. Where did this come from?


thanks.
-- 
~Randy

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Randy Dunlap]

On 11/03/16 23:53, Joe Perches wrote:
> On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
>> From: Madalin Bucur 
>> Date: Wed, 2 Nov 2016 22:17:26 +0200
>>
>>> This introduces the Freescale Data Path Acceleration Architecture
>>> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
>>> +{
>>> + u8 i;
>>> + size_t res = DPAA_BP_RAW_SIZE / 2;
>>
>> Always order local variable declarations from longest to shortest line,
>> also know as Reverse Christmas Tree Format.
> 
> I think this declaration sorting order is misguided but
> here's a possible change to checkpatch adding a test for it
> that does this test just for net/ and drivers/net/

I agree with the misguided part.
That's not actually in CodingStyle AFAICT. Where did this come from?


thanks.
-- 
~Randy

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Lino Sanfilippo]

Hi,

On 04.11.2016 07:53, Joe Perches wrote:


CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#446: FILE: drivers/net/ethernet/ethoc.c:446:
+   int size = bd.stat >> 16;
+   struct sk_buff *skb;



should not this case be valid? Optically the longer line is already before the 
shorter.
I think that the whole point in using this reverse xmas tree ordering is to have
the code optically tidied up and not to enforce ordering between variable name 
lengths.


Regards,
Lino

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Lino Sanfilippo]

Hi,

On 04.11.2016 07:53, Joe Perches wrote:


CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#446: FILE: drivers/net/ethernet/ethoc.c:446:
+   int size = bd.stat >> 16;
+   struct sk_buff *skb;



should not this case be valid? Optically the longer line is already before the 
shorter.
I think that the whole point in using this reverse xmas tree ordering is to have
the code optically tidied up and not to enforce ordering between variable name 
lengths.


Regards,
Lino

Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Joe Perches]

On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
> From: Madalin Bucur 
> Date: Wed, 2 Nov 2016 22:17:26 +0200
> 
> > This introduces the Freescale Data Path Acceleration Architecture
> > +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> > +{
> > + u8 i;
> > + size_t res = DPAA_BP_RAW_SIZE / 2;
> 
> Always order local variable declarations from longest to shortest line,
> also know as Reverse Christmas Tree Format.

I think this declaration sorting order is misguided but
here's a possible change to checkpatch adding a test for it
that does this test just for net/ and drivers/net/

(this likely won't apply because Evolution is a horrible
email client for sending patches, but the attachment should)

An example output:

$ ./scripts/checkpatch.pl -f drivers/net/ethernet/ethoc.c 
--types=reverse_xmas_tree --show-types
CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#200: FILE: drivers/net/ethernet/ethoc.c:200:
+   void __iomem *iobase;
+   void __iomem *membase;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#202: FILE: drivers/net/ethernet/ethoc.c:202:
+   int dma_alloc;
+   resource_size_t io_region_size;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#305: FILE: drivers/net/ethernet/ethoc.c:305:
+   int i;
+   void *vma;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#446: FILE: drivers/net/ethernet/ethoc.c:446:
+   int size = bd.stat >> 16;
+   struct sk_buff *skb;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#515: FILE: drivers/net/ethernet/ethoc.c:515:
+   int count;
+   struct ethoc_bd bd;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#675: FILE: drivers/net/ethernet/ethoc.c:675:
+   struct ethoc *priv = netdev_priv(dev);
+   struct phy_device *phy;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#756: FILE: drivers/net/ethernet/ethoc.c:756:
+   struct ethoc *priv = netdev_priv(dev);
+   struct mii_ioctl_data *mdio = if_mii(ifr);

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#801: FILE: drivers/net/ethernet/ethoc.c:801:
+   u32 mode = ethoc_read(priv, MODER);
+   struct netdev_hw_addr *ha;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#996: FILE: drivers/net/ethernet/ethoc.c:996:
+   struct resource *res = NULL;
+   struct resource *mmio = NULL;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#1001: FILE: drivers/net/ethernet/ethoc.c:1001:
+   int ret = 0;
+   bool random_mac = false;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#1002: FILE: drivers/net/ethernet/ethoc.c:1002:
+   bool random_mac = false;
+   struct ethoc_platform_data *pdata = dev_get_platdata(>dev);

total: 0 errors, 0 warnings, 11 checks, 1297 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
  mechanically convert to the typical style using --fix or --fix-inplace.

drivers/net/ethernet/ethoc.c has style problems, please review.

NOTE: Used message types: REVERSE_XMAS_TREE

NOTE: If any of the errors are false positives, please report
  them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
 scripts/checkpatch.pl | 70 ---
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fdacd759078e..dd344ac77cb8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2114,6 +2114,29 @@ sub pos_last_openparen {
    return length(expand_tabs(substr($line, 0, $last_openparen))) + 1;
 }
 
+sub get_decl {
+   my ($line) = @_;
+
+   # typical declarations
+   if ($line =~ /^\+\s+($Declare\s*$Ident)\s*[=,;:\[]/) {
+   return $1;
+   }
+   # function pointer declarations
+   if ($line =~ /^\+\s+($Declare\s*\(\s*\*\s*$Ident\s*\))\s*[=,;:\[\(]/) {
+   return $1;
+   }
+   # foo bar; where foo is some local typedef or #define
+   if ($line =~ /^\+\s+($Ident(?:\s+|\s*\*\s*)$Ident)\s*[=,;\[]/) {
+   return $1;
+   }
+   # known declaration macros
+   if ($line =~ /^\+\s+$(declaration_macros)/) {
+   return $1;
+   }
+
+   return undef;
+}
+
 sub process {
    my $filename = shift;
 
@@ -3063,9 +3086,7 @@ sub process {
    $last_blank_line = $linenr;
    }
 
-# check for missing blank lines after declarations
-   if ($sline =~ /^\+\s+\S/ && #Not at char 1
-   # actual declarations
+   my $prev_decl =
    ($prevline =~ /^\+\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
    # function pointer 

Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Joe Perches]

On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
> From: Madalin Bucur 
> Date: Wed, 2 Nov 2016 22:17:26 +0200
> 
> > This introduces the Freescale Data Path Acceleration Architecture
> > +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> > +{
> > + u8 i;
> > + size_t res = DPAA_BP_RAW_SIZE / 2;
> 
> Always order local variable declarations from longest to shortest line,
> also know as Reverse Christmas Tree Format.

I think this declaration sorting order is misguided but
here's a possible change to checkpatch adding a test for it
that does this test just for net/ and drivers/net/

(this likely won't apply because Evolution is a horrible
email client for sending patches, but the attachment should)

An example output:

$ ./scripts/checkpatch.pl -f drivers/net/ethernet/ethoc.c 
--types=reverse_xmas_tree --show-types
CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#200: FILE: drivers/net/ethernet/ethoc.c:200:
+   void __iomem *iobase;
+   void __iomem *membase;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#202: FILE: drivers/net/ethernet/ethoc.c:202:
+   int dma_alloc;
+   resource_size_t io_region_size;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#305: FILE: drivers/net/ethernet/ethoc.c:305:
+   int i;
+   void *vma;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#446: FILE: drivers/net/ethernet/ethoc.c:446:
+   int size = bd.stat >> 16;
+   struct sk_buff *skb;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#515: FILE: drivers/net/ethernet/ethoc.c:515:
+   int count;
+   struct ethoc_bd bd;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#675: FILE: drivers/net/ethernet/ethoc.c:675:
+   struct ethoc *priv = netdev_priv(dev);
+   struct phy_device *phy;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#756: FILE: drivers/net/ethernet/ethoc.c:756:
+   struct ethoc *priv = netdev_priv(dev);
+   struct mii_ioctl_data *mdio = if_mii(ifr);

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#801: FILE: drivers/net/ethernet/ethoc.c:801:
+   u32 mode = ethoc_read(priv, MODER);
+   struct netdev_hw_addr *ha;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#996: FILE: drivers/net/ethernet/ethoc.c:996:
+   struct resource *res = NULL;
+   struct resource *mmio = NULL;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#1001: FILE: drivers/net/ethernet/ethoc.c:1001:
+   int ret = 0;
+   bool random_mac = false;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#1002: FILE: drivers/net/ethernet/ethoc.c:1002:
+   bool random_mac = false;
+   struct ethoc_platform_data *pdata = dev_get_platdata(>dev);

total: 0 errors, 0 warnings, 11 checks, 1297 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
  mechanically convert to the typical style using --fix or --fix-inplace.

drivers/net/ethernet/ethoc.c has style problems, please review.

NOTE: Used message types: REVERSE_XMAS_TREE

NOTE: If any of the errors are false positives, please report
  them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
 scripts/checkpatch.pl | 70 ---
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fdacd759078e..dd344ac77cb8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2114,6 +2114,29 @@ sub pos_last_openparen {
    return length(expand_tabs(substr($line, 0, $last_openparen))) + 1;
 }
 
+sub get_decl {
+   my ($line) = @_;
+
+   # typical declarations
+   if ($line =~ /^\+\s+($Declare\s*$Ident)\s*[=,;:\[]/) {
+   return $1;
+   }
+   # function pointer declarations
+   if ($line =~ /^\+\s+($Declare\s*\(\s*\*\s*$Ident\s*\))\s*[=,;:\[\(]/) {
+   return $1;
+   }
+   # foo bar; where foo is some local typedef or #define
+   if ($line =~ /^\+\s+($Ident(?:\s+|\s*\*\s*)$Ident)\s*[=,;\[]/) {
+   return $1;
+   }
+   # known declaration macros
+   if ($line =~ /^\+\s+$(declaration_macros)/) {
+   return $1;
+   }
+
+   return undef;
+}
+
 sub process {
    my $filename = shift;
 
@@ -3063,9 +3086,7 @@ sub process {
    $last_blank_line = $linenr;
    }
 
-# check for missing blank lines after declarations
-   if ($sline =~ /^\+\s+\S/ && #Not at char 1
-   # actual declarations
+   my $prev_decl =
    ($prevline =~ /^\+\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
    # function pointer declarations
    

Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[David Miller]

From: Madalin Bucur 
Date: Wed, 2 Nov 2016 22:17:26 +0200

> This introduces the Freescale Data Path Acceleration Architecture
> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> +{
> + u8 i;
> + size_t res = DPAA_BP_RAW_SIZE / 2;

Always order local variable declarations from longest to shortest line,
also know as Reverse Christmas Tree Format.

Please audit your entire submission for this problem, it occurs
everywhere.

> + /* we do not want shared skbs on TX */
> + net_dev->priv_flags &= ~IFF_TX_SKB_SHARING;

Why?  By clearing this, you disallow an important fundamental way to do
performane testing, via pktgen.


> + int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
 ...
> + cpustats = (u64 *)_priv->stats;
> +
> + for (j = 0; j < numstats; j++)
> + netstats[j] += cpustats[j];

This is a memcpy() on well-typed datastructures which requires no
casting or special handling whatsoever, so use memcpy instead of
needlessly open coding the operation.

> +static int dpaa_change_mtu(struct net_device *net_dev, int new_mtu)
> +{
> + const int max_mtu = dpaa_get_max_mtu();
> +
> + /* Make sure we don't exceed the Ethernet controller's MAXFRM */
> + if (new_mtu < 68 || new_mtu > max_mtu) {
> + netdev_err(net_dev, "Invalid L3 mtu %d (must be between %d and 
> %d).\n",
> +new_mtu, 68, max_mtu);
> + return -EINVAL;
> + }
> + net_dev->mtu = new_mtu;
> +
> + return 0;
> +}

MTU restrictions are handled in the net-next tree via net_dev->min_mtu and
net_dev->max_mtu.  Use that and do not define this NDO operation as you do
not need it.

> +static int dpaa_set_features(struct net_device *dev, netdev_features_t 
> features)
> +{
> + /* Not much to do here for now */
> + dev->features = features;
> + return 0;
> +}

Do not define unnecessary NDO operations, let the defaults do their job.

> +static netdev_features_t dpaa_fix_features(struct net_device *dev,
> +netdev_features_t features)
> +{
> + netdev_features_t unsupported_features = 0;
> +
> + /* In theory we should never be requested to enable features that
> +  * we didn't set in netdev->features and netdev->hw_features at probe
> +  * time, but double check just to be on the safe side.
> +  */
> + unsupported_features |= NETIF_F_RXCSUM;
> +
> + features &= ~unsupported_features;
> +
> + return features;
> +}

Unless you can show that your need this, do not "guess" by implement this
NDO operation.  You don't need it.

> +#ifdef CONFIG_FSL_DPAA_ETH_FRIENDLY_IF_NAME
> +static int dpaa_mac_hw_index_get(struct platform_device *pdev)
> +{
> + struct device *dpaa_dev;
> + struct dpaa_eth_data *eth_data;
> +
> + dpaa_dev = >dev;
> + eth_data = dpaa_dev->platform_data;
> +
> + return eth_data->mac_hw_id;
> +}
> +
> +static int dpaa_mac_fman_index_get(struct platform_device *pdev)
> +{
> + struct device *dpaa_dev;
> + struct dpaa_eth_data *eth_data;
> +
> + dpaa_dev = >dev;
> + eth_data = dpaa_dev->platform_data;
> +
> + return eth_data->fman_hw_id;
> +}
> +#endif

Do not play network device naming games like this, use the standard name
assignment done by the kernel and have userspace entities do geographic or
device type specific naming.

I want to see this code completely removed.

> +static int dpaa_set_mac_address(struct net_device *net_dev, void *addr)
> +{
> + const struct dpaa_priv  *priv;
> + int err;
> + struct mac_device *mac_dev;
> +
> + priv = netdev_priv(net_dev);
> +
> + err = eth_mac_addr(net_dev, addr);
> + if (err < 0) {
> + netif_err(priv, drv, net_dev, "eth_mac_addr() = %d\n", err);
> + return err;
> + }
> +
> + mac_dev = priv->mac_dev;
> +
> + err = mac_dev->change_addr(mac_dev->fman_mac,
> +(enet_addr_t *)net_dev->dev_addr);
> + if (err < 0) {
> + netif_err(priv, drv, net_dev, "mac_dev->change_addr() = %d\n",
> +   err);
> + return err;
> + }

You MUST NOT return an error at this point without rewinding the state change
performed by eth_mac_addr().  Otherwise device will be left in an inconsistent
state compared to what the software MAC address has recorded.

This driver is enormous, I don't have the time nor the patience to
review it further for what seems to be many fundamental errors like
the ones I have pointed out so far.

Sorry.

Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet

[David Miller]

From: Madalin Bucur 
Date: Wed, 2 Nov 2016 22:17:26 +0200

> This introduces the Freescale Data Path Acceleration Architecture
> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> +{
> + u8 i;
> + size_t res = DPAA_BP_RAW_SIZE / 2;

Always order local variable declarations from longest to shortest line,
also know as Reverse Christmas Tree Format.

Please audit your entire submission for this problem, it occurs
everywhere.

> + /* we do not want shared skbs on TX */
> + net_dev->priv_flags &= ~IFF_TX_SKB_SHARING;

Why?  By clearing this, you disallow an important fundamental way to do
performane testing, via pktgen.


> + int numstats = sizeof(struct rtnl_link_stats64) / sizeof(u64);
 ...
> + cpustats = (u64 *)_priv->stats;
> +
> + for (j = 0; j < numstats; j++)
> + netstats[j] += cpustats[j];

This is a memcpy() on well-typed datastructures which requires no
casting or special handling whatsoever, so use memcpy instead of
needlessly open coding the operation.

> +static int dpaa_change_mtu(struct net_device *net_dev, int new_mtu)
> +{
> + const int max_mtu = dpaa_get_max_mtu();
> +
> + /* Make sure we don't exceed the Ethernet controller's MAXFRM */
> + if (new_mtu < 68 || new_mtu > max_mtu) {
> + netdev_err(net_dev, "Invalid L3 mtu %d (must be between %d and 
> %d).\n",
> +new_mtu, 68, max_mtu);
> + return -EINVAL;
> + }
> + net_dev->mtu = new_mtu;
> +
> + return 0;
> +}

MTU restrictions are handled in the net-next tree via net_dev->min_mtu and
net_dev->max_mtu.  Use that and do not define this NDO operation as you do
not need it.

> +static int dpaa_set_features(struct net_device *dev, netdev_features_t 
> features)
> +{
> + /* Not much to do here for now */
> + dev->features = features;
> + return 0;
> +}

Do not define unnecessary NDO operations, let the defaults do their job.

> +static netdev_features_t dpaa_fix_features(struct net_device *dev,
> +netdev_features_t features)
> +{
> + netdev_features_t unsupported_features = 0;
> +
> + /* In theory we should never be requested to enable features that
> +  * we didn't set in netdev->features and netdev->hw_features at probe
> +  * time, but double check just to be on the safe side.
> +  */
> + unsupported_features |= NETIF_F_RXCSUM;
> +
> + features &= ~unsupported_features;
> +
> + return features;
> +}

Unless you can show that your need this, do not "guess" by implement this
NDO operation.  You don't need it.

> +#ifdef CONFIG_FSL_DPAA_ETH_FRIENDLY_IF_NAME
> +static int dpaa_mac_hw_index_get(struct platform_device *pdev)
> +{
> + struct device *dpaa_dev;
> + struct dpaa_eth_data *eth_data;
> +
> + dpaa_dev = >dev;
> + eth_data = dpaa_dev->platform_data;
> +
> + return eth_data->mac_hw_id;
> +}
> +
> +static int dpaa_mac_fman_index_get(struct platform_device *pdev)
> +{
> + struct device *dpaa_dev;
> + struct dpaa_eth_data *eth_data;
> +
> + dpaa_dev = >dev;
> + eth_data = dpaa_dev->platform_data;
> +
> + return eth_data->fman_hw_id;
> +}
> +#endif

Do not play network device naming games like this, use the standard name
assignment done by the kernel and have userspace entities do geographic or
device type specific naming.

I want to see this code completely removed.

> +static int dpaa_set_mac_address(struct net_device *net_dev, void *addr)
> +{
> + const struct dpaa_priv  *priv;
> + int err;
> + struct mac_device *mac_dev;
> +
> + priv = netdev_priv(net_dev);
> +
> + err = eth_mac_addr(net_dev, addr);
> + if (err < 0) {
> + netif_err(priv, drv, net_dev, "eth_mac_addr() = %d\n", err);
> + return err;
> + }
> +
> + mac_dev = priv->mac_dev;
> +
> + err = mac_dev->change_addr(mac_dev->fman_mac,
> +(enet_addr_t *)net_dev->dev_addr);
> + if (err < 0) {
> + netif_err(priv, drv, net_dev, "mac_dev->change_addr() = %d\n",
> +   err);
> + return err;
> + }

You MUST NOT return an error at this point without rewinding the state change
performed by eth_mac_addr().  Otherwise device will be left in an inconsistent
state compared to what the software MAC address has recorded.

This driver is enormous, I don't have the time nor the patience to
review it further for what seems to be many fundamental errors like
the ones I have pointed out so far.

Sorry.