Re: [PATCH] hippi: fix spelling mistake: "Framming" -> "Framing"

[Jes Sorensen]

On 05/18/2018 06:09 AM, Colin King wrote:
> From: Colin Ian King 
> 
> Trivial fix to spelling mistake in printk message text
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/hippi/rrunner.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/hippi/rrunner.c b/drivers/net/hippi/rrunner.c
> index 1ab97d99b9ba..f41116488079 100644
> --- a/drivers/net/hippi/rrunner.c
> +++ b/drivers/net/hippi/rrunner.c
> @@ -867,7 +867,7 @@ static u32 rr_handle_event(struct net_device *dev, u32 
> prodidx, u32 eidx)
>  dev->name);
>   goto drop;
>   case E_FRM_ERR:
> - printk(KERN_WARNING "%s: Framming Error\n",
> + printk(KERN_WARNING "%s: Framing Error\n",
>  dev->name);
>   goto drop;
>   case E_FLG_SYN_ERR:
> 

Fine with me! I do wonder if it's time to retire this driver and the
HIPPI code. I haven't had access to hardware for over a decade so I have
no idea if it even works anymore.

Jes

Re: [PATCH] rtl8xxxu: Fix trailing semicolon

[Jes Sorensen]

On 01/17/2018 05:56 AM, Luis de Bethencourt wrote:
> The trailing semicolon is an empty statement that does no operation.
> Removing it since it doesn't do anything.
> 
> Signed-off-by: Luis de Bethencourt <lui...@kernel.org>
> ---
> 
> Hi,
> 
> After fixing the same thing in drivers/staging/rtl8723bs/, Joe Perches
> suggested I fix it treewide [0].
> 
> Best regards 
> Luis
> 
> 
> [0] 
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115410.html
> [1] 
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115390.html
> 
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Jes Sorensen <jes.soren...@gmail.com>


> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index 95e3993d8a33..8828baf26e7b 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -1172,7 +1172,7 @@ struct rtl8723bu_c2h {
>  
>   u8 basic_rate:1;
>   u8 bt_has_reset:1;
> - u8 dummy4_1:1;;
> + u8 dummy4_1:1;
>   u8 ignore_wlan:1;
>   u8 auto_report:1;
>   u8 dummy4_2:3;
> 

Re: [net 02/14] Revert "mlx5: move affinity hints assignments to generic code"

[Jes Sorensen]

On 12/19/2017 05:24 PM, Saeed Mahameed wrote:
> Before the offending commit, mlx5 core did the IRQ affinity itself,
> and it seems that the new generic code have some drawbacks and one
> of them is the lack for user ability to modify irq affinity after
> the initial affinity values got assigned.
> 
> The issue is still being discussed and a solution in the new generic code
> is required, until then we need to revert this patch.
> 
> This fixes the following issue:
> echo  > /proc/irq//smp_affinity
> fails with  -EIO
> 
> This reverts commit a435393acafbf0ecff4deb3e3cb554b34f0d0664.
> Note: kept mlx5_get_vector_affinity in include/linux/mlx5/driver.h since
> it is used in mlx5_ib driver.
> 
> Fixes: a435393acafb ("mlx5: move affinity hints assignments to generic code")
> Cc: Sagi Grimberg <s...@grimberg.me>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Jes Sorensen <jsoren...@fb.com>
> Reported-by: Jes Sorensen <jsoren...@fb.com>
> Signed-off-by: Saeed Mahameed <sae...@mellanox.com>

Acked-by: Jes Sorensen <jsoren...@fb.com>

Cheers,
Jes


> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en.h  |  1 +
>  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 45 +++---
>  drivers/net/ethernet/mellanox/mlx5/core/main.c| 75 
> +--
>  include/linux/mlx5/driver.h   |  1 +
>  4 files changed, 93 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h 
> b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> index c0872b3284cb..43f9054830e5 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -590,6 +590,7 @@ struct mlx5e_channel {
>   struct mlx5_core_dev  *mdev;
>   struct hwtstamp_config*tstamp;
>   intix;
> + intcpu;
>  };
>  
>  struct mlx5e_channels {
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c 
> b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index d2b057a3e512..cbec66bc82f1 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -71,11 +71,6 @@ struct mlx5e_channel_param {
>   struct mlx5e_cq_param  icosq_cq;
>  };
>  
> -static int mlx5e_get_node(struct mlx5e_priv *priv, int ix)
> -{
> - return pci_irq_get_node(priv->mdev->pdev, MLX5_EQ_VEC_COMP_BASE + ix);
> -}
> -
>  static bool mlx5e_check_fragmented_striding_rq_cap(struct mlx5_core_dev 
> *mdev)
>  {
>   return MLX5_CAP_GEN(mdev, striding_rq) &&
> @@ -444,17 +439,16 @@ static int mlx5e_rq_alloc_mpwqe_info(struct mlx5e_rq 
> *rq,
>   int wq_sz = mlx5_wq_ll_get_size(>wq);
>   int mtt_sz = mlx5e_get_wqe_mtt_sz();
>   int mtt_alloc = mtt_sz + MLX5_UMR_ALIGN - 1;
> - int node = mlx5e_get_node(c->priv, c->ix);
>   int i;
>  
>   rq->mpwqe.info = kzalloc_node(wq_sz * sizeof(*rq->mpwqe.info),
> - GFP_KERNEL, node);
> +   GFP_KERNEL, cpu_to_node(c->cpu));
>   if (!rq->mpwqe.info)
>   goto err_out;
>  
>   /* We allocate more than mtt_sz as we will align the pointer */
> - rq->mpwqe.mtt_no_align = kzalloc_node(mtt_alloc * wq_sz,
> - GFP_KERNEL, node);
> + rq->mpwqe.mtt_no_align = kzalloc_node(mtt_alloc * wq_sz, GFP_KERNEL,
> + cpu_to_node(c->cpu));
>   if (unlikely(!rq->mpwqe.mtt_no_align))
>   goto err_free_wqe_info;
>  
> @@ -562,7 +556,7 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
>   int err;
>   int i;
>  
> - rqp->wq.db_numa_node = mlx5e_get_node(c->priv, c->ix);
> + rqp->wq.db_numa_node = cpu_to_node(c->cpu);
>  
>   err = mlx5_wq_ll_create(mdev, >wq, rqc_wq, >wq,
>   >wq_ctrl);
> @@ -629,8 +623,7 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
>   default: /* MLX5_WQ_TYPE_LINKED_LIST */
>   rq->wqe.frag_info =
>   kzalloc_node(wq_sz * sizeof(*rq->wqe.frag_info),
> -  GFP_KERNEL,
> -  mlx5e_get_node(c->priv, c->ix));
> +  GFP_KERNEL, cpu_to_node(c->cpu));
>   if (!rq->wqe.frag_info) {
>   err = -ENOMEM;
>   goto err_rq_wq_destroy;
> @@ -1000,13 +993,13 @@ static int mlx5e_alloc_xdpsq(struct mlx5e_channel *c,
>   sq-&

Re: Wifi RTL8723bu driver test: failed to scan

[Jes Sorensen]

On 11/22/2017 04:51 AM, Mylene JOSSERAND wrote:
> Hello Jes Sorensen,
> 
> I am currently testing a LM811 Wifi/BT USB dongle [1] on a Sinlinx
> SinA33 Allwinner SoC board [2]. I saw that I should use the realtek
> driver RTL8723BU for this USB dongle.
> 
> Currently, I am only testing the Wifi and the mainline driver (kernel
> 4.14-rc7) does not seem to work. At least, the scanning does not output
> anything.
> 
> I tested the driver recommended by LM Technologies [3] and it works
> fine (scan, connect and ping are ok). Before investigating on the
> differences between these two drivers, do you have any idea about this
> issue?
> 
> Here are the commands and output I got with mainline's driver:

I have not looked at the driver these LM Technologies people are
referring to, but I am guessing it's the vendor code.

8723bu is a little dicey because it has BT in the chip and if you enable
that the two drivers need to interact, which rtl8xxxu currently doesn't
know about. Check your dmesg output to make sure you don't have some BT
thing loaded hijacking the chip.

Jes

Re: mlx5 broken affinity

[Jes Sorensen]

On 11/08/2017 12:33 PM, Thomas Gleixner wrote:
> On Wed, 8 Nov 2017, Jes Sorensen wrote:
>> On 11/07/2017 10:07 AM, Thomas Gleixner wrote:
>>> Depending on the machine and the number of queues this might even result in
>>> completely losing the ability to suspend/hibernate because the number of
>>> available vectors on CPU0 is not sufficient to accomodate all queue
>>> interrupts.
>>
>> Depending on the system, suspend/resume is really the lesser interesting
>> issue to the user. Pretty much any system with a 10/25GBps mlx5 NIC in
>> it will be in some sort of rack and is unlikely to ever want to
>> suspend/resume.
> 
> The discussions with Intel about that tell a different story and cpu
> online/offline for power management purposes is - while debatable - widely
> used.

I certainly do not want to dispute that, it just underlines that
different users have different priorities.

>>> A lot of things are possible, the question is whether it makes sense. The
>>> whole point is to have resources (queues, interrupts etc.) per CPU and have
>>> them strictly associated.
>>>
>>> Why would you give the user a knob to destroy what you carefully optimized?
>>>
>>> Just because we can and just because users love those knobs or is there any
>>> real technical reason?
>>
>> Because the user sometimes knows better based on statically assigned
>> loads, or the user wants consistency across kernels. It's great that the
>> system is better at allocating this now, but we also need to allow for a
>> user to change it. Like anything on Linux, a user wanting to blow off
>> his/her own foot, should be allowed to do so.
> 
> That's fine, but that's not what the managed affinity facility provides. If
> you want to leverage the spread mechanism, but avoid the managed part, then
> this is a different story and we need to figure out how to provide that
> without breaking the managed side of it.
> 
> As I said it's possible, but I vehemently disagree, that this is a bug in
> the core code, as it was claimed several times in this thread.

So it may be my original message was confusing on this. Currently
IRQ-affinity.txt describes how a user can change that, but it no longer
works if an IRQ is marked managed. That is what I qualified as a bug in
my original posting. If an IRQ is not meant to have it's affinity
modified because it is managed, then I think it should also result in
the permissions on the /proc file change to rrr rather than getting EIO
when trying to write to it, but that is a minor detail IMHO.

None of this is a major showstopper for the next kernel release, as long
as we can work on a longer term fix that satisfies everyone.

What I would ideally like to see is the option for drivers to benefit
from the new allocation scheme without being locked in and also have the
option to go managed if that is right for the given devices. Basically a
best of both Worlds situation.

> The real issue is that the driver was converted to something which was
> expected to behave differently. That's hardly a bug in the core code, at
> most it's a documentation problem.

When I hit this issue I read the driver commit stating it was taking
advantage of the new allocation. Not having been part of the discussion
and design of the new core code, I just caught what was described in the
commit message for mlx5.

For now I have just reverted the offending mlx5 patch in our kernel
while we figure out how to resolve it long term.

Cheers,
Jes

Re: mlx5 broken affinity

[Jes Sorensen]

On 11/07/2017 10:07 AM, Thomas Gleixner wrote:
> On Sun, 5 Nov 2017, Sagi Grimberg wrote:
>> I do agree that the user would lose better cpu online/offline behavior,
>> but it seems that users want to still have some control over the IRQ
>> affinity assignments even if they lose this functionality.
> 
> Depending on the machine and the number of queues this might even result in
> completely losing the ability to suspend/hibernate because the number of
> available vectors on CPU0 is not sufficient to accomodate all queue
> interrupts.

Depending on the system, suspend/resume is really the lesser interesting
issue to the user. Pretty much any system with a 10/25GBps mlx5 NIC in
it will be in some sort of rack and is unlikely to ever want to
suspend/resume.

>> Would it be possible to keep the managed facility until a user overrides
>> an affinity assignment? This way if the user didn't touch it, we keep
>> all the perks, and in case the user overrides it, we log the implication
>> so the user is aware?
> 
> A lot of things are possible, the question is whether it makes sense. The
> whole point is to have resources (queues, interrupts etc.) per CPU and have
> them strictly associated.
> 
> Why would you give the user a knob to destroy what you carefully optimized?
> 
> Just because we can and just because users love those knobs or is there any
> real technical reason?

Because the user sometimes knows better based on statically assigned
loads, or the user wants consistency across kernels. It's great that the
system is better at allocating this now, but we also need to allow for a
user to change it. Like anything on Linux, a user wanting to blow off
his/her own foot, should be allowed to do so.

Cheers,
Jes

Re: mlx5 broken affinity

[Jes Sorensen]

On 11/02/2017 12:14 PM, Sagi Grimberg wrote:
> 
>> This wasn't to start a debate about which allocation method is the
>> perfect solution. I am perfectly happy with the new default, the part
>> that is broken is to take away the user's option to reassign the
>> affinity. That is a bug and it needs to be fixed!
> 
> Well,
> 
> I would really want to wait for Thomas/Christoph to reply, but this
> simple change fixed it for me:
> -- 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 573dc52b0806..eccd06be5e44 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -146,8 +146,7 @@ bool irq_can_set_affinity_usr(unsigned int irq)
>  {
>     struct irq_desc *desc = irq_to_desc(irq);
> 
> -   return __irq_can_set_affinity(desc) &&
> -   !irqd_affinity_is_managed(>irq_data);
> +   return __irq_can_set_affinity(desc);
>  }

The part I don't get here is why the mlx5 driver change was allowed to
set the irqs to 'managed'? One thing is to use the generic system for
initial allocation, which is all great, but the change shouldn't mark
the irq affinity mapping as managed.

Jes

Re: mlx5 broken affinity

[Jes Sorensen]

On 11/02/2017 06:08 AM, Sagi Grimberg wrote:
> 
 I vaguely remember Nacking Sagi's patch as we knew it would break
 mlx5e netdev affinity assumptions.
>> I remember that argument. Still the series found its way in.
> 
> Of course it maid its way in, it was acked by three different
> maintainers, and I addressed all of Saeed's comments.
> 
>> That series moves affinity decisions to kernel's responsibility.
>> AFAI see, what kernel does is assign IRQs to the NUMA's one by one in
>> increasing indexing (starting with cores of NUMA #0), no matter what
>> NUMA is closer to the NIC.
> 
> Well, as we said before, if there is a good argument to do the home node
> first we can change the generic code (as it should be given that this is
> absolutely not device specific).
> 
>> This means that if your NIC is on NUMA #1, and you reduce the number
>> of channels, you might end up working only with the cores on the far
>> NUMA. Not good!
> We deliberated on this before, and concluded that application affinity
> and device affinity are equally important. If you have a real use case
> that shows otherwise, its perfectly doable to start from the device home
> node.

This wasn't to start a debate about which allocation method is the
perfect solution. I am perfectly happy with the new default, the part
that is broken is to take away the user's option to reassign the
affinity. That is a bug and it needs to be fixed!

Jes

Re: mlx5 broken affinity

[Jes Sorensen]

On 11/01/2017 06:41 PM, Saeed Mahameed wrote:
> On Wed, Nov 1, 2017 at 11:20 AM, Jes Sorensen <jsoren...@fb.com> wrote:
>> On 11/01/2017 01:21 PM, Sagi Grimberg wrote:
>> I am all in favor of making the automatic setup better, but assuming an
>> automatic setup is always right seems problematic. There could be
>> workloads where you may want to assign affinity explicitly.
>>
>> Jes
> 
> I vaguely remember Nacking Sagi's patch as we knew it would break
> mlx5e netdev affinity assumptions.
> Anyway we already submitted the mlx5e patch that removed such assumption
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_809196_=DwIBaQ=5VD0RTtNlTh3ycd41b3MUw=zRrmoWoylV2tnG53v9ZA2w=Z6xtsiQVL8xhTauY_DrOWYDhci-D49TqNKLWV_HK5Ug=pkxagNCZzy5-ZzMRTxIQ5pDfFq8WOSRdSx5zeQpQdBI=
>  ("net/mlx5e: Distribute RSS
> table among all RX rings")
> Jes please confirm you have it.
> 
> And I agree here that user should be able to read
> /proc/irq/x/smp_affinity and even modify it if required.

Hi Saeed,

I can confirm I have that patch - the problem is also present in Linus'
current tree.

I can read smp_affinity, but I cannot write to it.

Cheers,
Jes

Re: mlx5 broken affinity

[Jes Sorensen]

On 11/01/2017 01:21 PM, Sagi Grimberg wrote:
>> Hi,
> 
> Hi Jes,
> 
>> The below patch seems to have broken PCI IRQ affinity assignments for
>> mlx5.
> 
> I wouldn't call it breaking IRQ affinity assignments. It just makes
> them automatic.

Well it explicitly breaks the option for an admin to assign them
manually, which contradicts what is written in
Documentation/IRQ-affinity.txt

>> Prior to this patch I could echo a value to /proc/irq//smp_affinity
>> and it would get assigned. With this patch applied I get -EIO
> 
> Adding Christoph,
> 
> Ideally the affinity assignments would be better left alone, but
> I wasn't aware that they are now immutable. Christoph?
> 
>> The actual affinity assignments seem to have changed too, but I assume
>> this is a result of the generic allocation?
> 
> The affinity assignments should be giving you perfect linear assignment
> of the rx/tx rings completion vectors to CPU cores:
> first  -> 0
> second -> 1
> third  -> 2
> ...
> 
> Before, mlx5 spread affinity starting from the local numa node as it
> relied on that when constructing RSS indirection table only to the local
> numa node (which as a side effect hurt applications running on the far
> node as the traffic was guaranteed to arrive rx rings located in the
> "wrong" node).
> 
> Now the RSS indirection table is linear across both numa nodes just like
> the irq affinity settings. Another thing that was improved was the
> pre/post vectors which blacklisted any non rx/tx completion vectors from
> the affinity assignments (like port events completion vectors from
> example).

I am all in favor of making the automatic setup better, but assuming an
automatic setup is always right seems problematic. There could be
workloads where you may want to assign affinity explicitly.

Jes

mlx5 broken affinity

[Jes Sorensen]

Hi,

The below patch seems to have broken PCI IRQ affinity assignments for mlx5.

Prior to this patch I could echo a value to /proc/irq//smp_affinity
and it would get assigned. With this patch applied I get -EIO

The actual affinity assignments seem to have changed too, but I assume
this is a result of the generic allocation?

With this applied I get:
[root@testbox /proc/irq]# cat 50/smp_affinity
00,0010
[root@textbox /proc/irq]# cat 100/smp_affinity
10,

Without I get:
[root@testbox /proc/irq]# cat 50/smp_affinity
00,0200
[root@testbox /proc/irq]# cat 100/smp_affinity
000100,

I am not wildly familiar with the affinity assignment code. Is there
something obvious I am missing here?

Cheers,
Jes


commit a435393acafbf0ecff4deb3e3cb554b34f0d0664
Author: Sagi Grimberg 
Date:   Thu Jul 13 11:09:40 2017 +0300

mlx5: move affinity hints assignments to generic code

generic api takes care of spreading affinity similar to
what mlx5 open coded (and even handles better asymmetric
configurations). Ask the generic API to spread affinity
for us, and feed him pre_vectors that do not participate
in affinity settings (which is an improvement to what we
had before).

The affinity assignments should match what mlx5 tried to
do earlier but now we do not set affinity to async, cmd
and pages dedicated vectors.

Also, remove mlx5e_get_cpu and introduce mlx5e_get_node
(used for allocation purposes) and mlx5_get_vector_affinity
(for indirection table construction) as they provide the needed
information. Luckily, we have generic helpers to get cpumask
and node given a irq vector. mlx5_get_vector_affinity will
be used by mlx5_ib in a subsequent patch.

Reviewed-by: Christoph Hellwig 
Acked-by: Leon Romanovsky 
Signed-off-by: Sagi Grimberg 
Signed-off-by: Doug Ledford 

Re: [PATCH] drivers/net: hippi: Convert timers to use timer_setup()

[Jes Sorensen]

On 10/25/2017 06:51 AM, Kees Cook wrote:

In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.

Cc: Jes Sorensen <j...@trained-monkey.org>
Cc: linux-hi...@sunsite.dk
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
  drivers/net/hippi/rrunner.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)


Looks good to me.

Jes


diff --git a/drivers/net/hippi/rrunner.c b/drivers/net/hippi/rrunner.c
index 76cc140774a2..8483f03d5a41 100644
--- a/drivers/net/hippi/rrunner.c
+++ b/drivers/net/hippi/rrunner.c
@@ -1146,10 +1146,10 @@ static inline void rr_raz_rx(struct rr_private *rrpriv,
}
  }
  
-static void rr_timer(unsigned long data)

+static void rr_timer(struct timer_list *t)
  {
-   struct net_device *dev = (struct net_device *)data;
-   struct rr_private *rrpriv = netdev_priv(dev);
+   struct rr_private *rrpriv = from_timer(rrpriv, t, timer);
+   struct net_device *dev = pci_get_drvdata(rrpriv->pci_dev);
struct rr_regs __iomem *regs = rrpriv->regs;
unsigned long flags;
  
@@ -1229,7 +1229,7 @@ static int rr_open(struct net_device *dev)
  
  	/* Set the timer to switch to check for link beat and perhaps switch

   to an alternate media type. */
-   setup_timer(>timer, rr_timer, (unsigned long)dev);
+   timer_setup(>timer, rr_timer, 0);
rrpriv->timer.expires = RUN_AT(5*HZ);   /* 5 sec. watchdog */
add_timer(>timer);
  

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Jes Sorensen]

On 10/11/2017 04:41 AM, Kalle Valo wrote:

Jes Sorensen <jes.soren...@gmail.com> writes:


On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.


While this isn't harmful, to me this looks like pointless patch churn
for zero gain and it's just ugly.


In general I find it useful to mark fall through cases. And it's just a
comment with two words, so they cannot hurt your eyes that much.


I don't see them being harmful in the code, but I don't see them of much 
use either. If it happened as part of natural code development, fine. My 
objection is to people running around doing this systematically causing 
patch churn for little to zero gain.


Jes

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Jes Sorensen]

On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.


While this isn't harmful, to me this looks like pointless patch churn 
for zero gain and it's just ugly.


Jes



Cc: Jes Sorensen <jes.soren...@gmail.com>
Cc: Kalle Valo <kv...@codeaurora.org>
Cc: linux-wirel...@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
---
  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 7806a4d..e66be05 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -1153,6 +1153,7 @@ void rtl8xxxu_gen1_config_channel(struct ieee80211_hw *hw)
switch (hw->conf.chandef.width) {
case NL80211_CHAN_WIDTH_20_NOHT:
ht = false;
+   /* fall through */
case NL80211_CHAN_WIDTH_20:
opmode |= BW_OPMODE_20MHZ;
rtl8xxxu_write8(priv, REG_BW_OPMODE, opmode);
@@ -1280,6 +1281,7 @@ void rtl8xxxu_gen2_config_channel(struct ieee80211_hw *hw)
switch (hw->conf.chandef.width) {
case NL80211_CHAN_WIDTH_20_NOHT:
ht = false;
+   /* fall through */
case NL80211_CHAN_WIDTH_20:
rf_mode_bw |= WMAC_TRXPTCL_CTL_BW_20;
subchannel = 0;
@@ -1748,9 +1750,11 @@ static int rtl8xxxu_identify_chip(struct rtl8xxxu_priv 
*priv)
case 3:
priv->ep_tx_low_queue = 1;
priv->ep_tx_count++;
+   /* fall through */
case 2:
priv->ep_tx_normal_queue = 1;
priv->ep_tx_count++;
+   /* fall through */
case 1:
priv->ep_tx_high_queue = 1;
priv->ep_tx_count++;
@@ -5691,6 +5695,7 @@ static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum 
set_key_cmd cmd,
break;
case WLAN_CIPHER_SUITE_TKIP:
key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
+   /* fall through */
default:
return -EOPNOTSUPP;
}

Re: [PATCH RFC net-next 4/4] net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

[Jes Sorensen]

On 06/13/2017 01:58 PM, Saeed Mahameed wrote:

On Mon, Jun 12, 2017 at 9:20 PM, Jes Sorensen <jsoren...@fb.com> wrote:

On 06/07/2017 07:42 PM, Saeed Mahameed wrote:


This patch gives the option to chose whether to compile the driver with or
without eswitch/eswitch_offloads(switchdev mode)/en_rep(VF representors)
and en_tc offloads.

It also removes most of the above modules headers declarations and stubs
out the API functions which are used outside these modules.

Signed-off-by: Saeed Mahameed <sae...@mellanox.com>
---
   drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  7 +
   drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  6 +++--
   drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 33
+++
   drivers/net/ethernet/mellanox/mlx5/core/en_rep.h  |  8 ++
   drivers/net/ethernet/mellanox/mlx5/core/en_rx.c   |  2 ++
   drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |  7 +
   drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 23 +++-
   drivers/net/ethernet/mellanox/mlx5/core/main.c| 10 +--
   8 files changed, 68 insertions(+), 28 deletions(-)



Overall good, a few nits



@@ -3316,6 +3317,7 @@ static int mlx5e_ioctl(struct net_device *dev,
struct ifreq *ifr, int cmd)
 }
   }
   +#ifdef CONFIG_MLX5_ESWITCH
   static int mlx5e_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
   {
 struct mlx5e_priv *priv = netdev_priv(dev);
@@ -3418,6 +3420,7 @@ static int mlx5e_get_vf_stats(struct net_device
*dev,
 return mlx5_eswitch_get_vport_stats(mdev->priv.eswitch, vf + 1,
 vf_stats);
   }
+#endif
 static void mlx5e_add_vxlan_port(struct net_device *netdev,
  struct udp_tunnel_info *ti)
@@ -3659,6 +3662,7 @@ static const struct net_device_ops
mlx5e_netdev_ops_basic = {
   #endif
   };
   +#ifdef CONFIG_MLX5_ESWITCH
   static const struct net_device_ops mlx5e_netdev_ops_sriov = {
 .ndo_open= mlx5e_open,
 .ndo_stop= mlx5e_close,
@@ -3697,6 +3701,7 @@ static const struct net_device_ops
mlx5e_netdev_ops_sriov = {
 .ndo_has_offload_stats   = mlx5e_has_offload_stats,
 .ndo_get_offload_stats   = mlx5e_get_offload_stats,
   };
+#endif
 static int mlx5e_check_required_hca_cap(struct mlx5_core_dev *mdev)
   {
@@ -3923,9 +3928,11 @@ static void mlx5e_set_netdev_dev_addr(struct
net_device *netdev)
 }
   }
   +#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
   static const struct switchdev_ops mlx5e_switchdev_ops = {
 .switchdev_port_attr_get= mlx5e_attr_get,
   };
+#endif
 static void mlx5e_build_nic_netdev(struct net_device *netdev)
   {



Why not move these functions and the struct into one of the files that is
being compiled out. The less #ifdefs we leave in the code the better.



eswitch is independent from netdev, and we want to keep netdev ndos
local to netdev files.


Not the end of the World then.


@@ -3936,15 +3943,17 @@ static void mlx5e_build_nic_netdev(struct
net_device *netdev)
 SET_NETDEV_DEV(netdev, >pdev->dev);
   - if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
-   netdev->netdev_ops = _netdev_ops_sriov;
   #ifdef CONFIG_MLX5_CORE_EN_DCB
-   if (MLX5_CAP_GEN(mdev, qos))
-   netdev->dcbnl_ops = _dcbnl_ops;
+   if (MLX5_CAP_GEN(mdev, vport_group_manager) && MLX5_CAP_GEN(mdev,
qos))
+   netdev->dcbnl_ops = _dcbnl_ops;
+#endif
+
+#ifdef CONFIG_MLX5_ESWITCH
+   if (MLX5_CAP_GEN(mdev, vport_group_manager))
+   netdev->netdev_ops = _netdev_ops_sriov;
+   else
   #endif
-   } else {
 netdev->netdev_ops = _netdev_ops_basic;
-   }
 netdev->watchdog_timeo= 15 * HZ;



This kind of #ifdef is always bad, it's hard to read and easy to get wrong.
Why not have MLX5_CAP_GEN return 0 if MLX5_ESWITCH is not enabled and have a
dummy pointer?



i know ifdef is ugly, but we have to provide basic ndos (not dummy
pointers) when eswitch is not avaialbe, also we want the code to be as
much as free from empty functions that all they do is to return
-EOPNOTSUPP;

for my taste this way is cleaner and more readable, from these line
you can understand when SRIOV/Eswitch is not supported. you don't need
to backtrack the function pointers.


This is not a good way of doing it, with #ifdef's mangling an if() 
statement. It really should be avoided. Any problem having 
MLX5_CAP_GEN() return 0 when ESWITCH is not enabled?



@@ -4016,7 +4025,7 @@ static void mlx5e_build_nic_netdev(struct net_device
*netdev)
 mlx5e_set_netdev_dev_addr(netdev);
   -#ifdef CONFIG_NET_SWITCHDEV
+#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
 if (MLX5_CAP_GEN(mdev, vport_group_manager))
 netdev->switchdev_ops = _switchdev_ops;
   #

Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Jes Sorensen]

On 06/13/2017 01:49 PM, Saeed Mahameed wrote:

On Mon, Jun 12, 2017 at 8:52 PM, Jes Sorensen <jsoren...@fb.com> wrote:

On 06/07/2017 07:42 PM, Saeed Mahameed wrote:


Multi-Physical Function Switch (MPFs) is required for when multi-PF
configuration is enabled to allow passing user configured unicast MAC
addresses to the requesting PF.

Before this patch eswitch.c used to manage the HW MPFS l2 table,
eswitch always enabled vport(0) (NIC PF) vport's contexts update on
unicast
mac address list changes, to populate the PF's MPFS L2 table accordingly,
even if SRIOV was not enabled.

In downstream patch we would like to allow compiling the driver without
eswitch functionalities, for that we move MPFS l2 table logic out
of eswitch.c into its own file, and provide Kconfig flag (MLX5_MPFS) to
allow compiling out MPFS for those who don't want Multi-PF support

NIC PF netdevice will now directly update MPFS l2 table via the new MPFS
API. VF netdevice has no access to MPFS L2 table, so e-Switch will remain
responsible of updating its PF MPFS l2 table on behalf of its VFs.

Due to this change we also don't require enabling vport(0) (PF vport)
unicast mac changes events anymore, for when SRIOV is not enabled.
Which means eswitch is now activated only on SRIOV activation, and not
required otherwise.




On overall it looks good - one nit.


+static int alloc_l2table_index(struct mlx5_mpfs *l2table, u32 *ix)
+{
+   int err = 0;
+
+   *ix = find_first_zero_bit(l2table->bitmap, l2table->size);
+   if (*ix >= l2table->size)
+   err = -ENOSPC;
+   else
+   __set_bit(*ix, l2table->bitmap);
+
+   return err;
+}



You pass in a pointer for ix but you don't modify it, why not just pass in
the value?.



we do modify ix:
*ix = find_first_zero_bit(l2table->bitmap, l2table->size);

The idea is to find the next free index and return it to the caller


I clearly need new glasses :(

Jes

Re: [PATCH RFC net-next 4/4] net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

[Jes Sorensen]

On 06/07/2017 07:42 PM, Saeed Mahameed wrote:

This patch gives the option to chose whether to compile the driver with or
without eswitch/eswitch_offloads(switchdev mode)/en_rep(VF representors)
and en_tc offloads.

It also removes most of the above modules headers declarations and stubs
out the API functions which are used outside these modules.

Signed-off-by: Saeed Mahameed 
---
  drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  7 +
  drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  6 +++--
  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 33 +++
  drivers/net/ethernet/mellanox/mlx5/core/en_rep.h  |  8 ++
  drivers/net/ethernet/mellanox/mlx5/core/en_rx.c   |  2 ++
  drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |  7 +
  drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 23 +++-
  drivers/net/ethernet/mellanox/mlx5/core/main.c| 10 +--
  8 files changed, 68 insertions(+), 28 deletions(-)


Overall good, a few nits


@@ -3316,6 +3317,7 @@ static int mlx5e_ioctl(struct net_device *dev, struct 
ifreq *ifr, int cmd)
}
  }
  
+#ifdef CONFIG_MLX5_ESWITCH

  static int mlx5e_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
  {
struct mlx5e_priv *priv = netdev_priv(dev);
@@ -3418,6 +3420,7 @@ static int mlx5e_get_vf_stats(struct net_device *dev,
return mlx5_eswitch_get_vport_stats(mdev->priv.eswitch, vf + 1,
vf_stats);
  }
+#endif
  
  static void mlx5e_add_vxlan_port(struct net_device *netdev,

 struct udp_tunnel_info *ti)
@@ -3659,6 +3662,7 @@ static const struct net_device_ops mlx5e_netdev_ops_basic 
= {
  #endif
  };
  
+#ifdef CONFIG_MLX5_ESWITCH

  static const struct net_device_ops mlx5e_netdev_ops_sriov = {
.ndo_open= mlx5e_open,
.ndo_stop= mlx5e_close,
@@ -3697,6 +3701,7 @@ static const struct net_device_ops mlx5e_netdev_ops_sriov 
= {
.ndo_has_offload_stats   = mlx5e_has_offload_stats,
.ndo_get_offload_stats   = mlx5e_get_offload_stats,
  };
+#endif
  
  static int mlx5e_check_required_hca_cap(struct mlx5_core_dev *mdev)

  {
@@ -3923,9 +3928,11 @@ static void mlx5e_set_netdev_dev_addr(struct net_device 
*netdev)
}
  }
  
+#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)

  static const struct switchdev_ops mlx5e_switchdev_ops = {
.switchdev_port_attr_get= mlx5e_attr_get,
  };
+#endif
  
  static void mlx5e_build_nic_netdev(struct net_device *netdev)

  {


Why not move these functions and the struct into one of the files that 
is being compiled out. The less #ifdefs we leave in the code the better.



@@ -3936,15 +3943,17 @@ static void mlx5e_build_nic_netdev(struct net_device 
*netdev)
  
  	SET_NETDEV_DEV(netdev, >pdev->dev);
  
-	if (MLX5_CAP_GEN(mdev, vport_group_manager)) {

-   netdev->netdev_ops = _netdev_ops_sriov;
  #ifdef CONFIG_MLX5_CORE_EN_DCB
-   if (MLX5_CAP_GEN(mdev, qos))
-   netdev->dcbnl_ops = _dcbnl_ops;
+   if (MLX5_CAP_GEN(mdev, vport_group_manager) && MLX5_CAP_GEN(mdev, qos))
+   netdev->dcbnl_ops = _dcbnl_ops;
+#endif
+
+#ifdef CONFIG_MLX5_ESWITCH
+   if (MLX5_CAP_GEN(mdev, vport_group_manager))
+   netdev->netdev_ops = _netdev_ops_sriov;
+   else
  #endif
-   } else {
netdev->netdev_ops = _netdev_ops_basic;
-   }
  
  	netdev->watchdog_timeo= 15 * HZ;


This kind of #ifdef is always bad, it's hard to read and easy to get 
wrong. Why not have MLX5_CAP_GEN return 0 if MLX5_ESWITCH is not enabled 
and have a dummy pointer?



@@ -4016,7 +4025,7 @@ static void mlx5e_build_nic_netdev(struct net_device 
*netdev)
  
  	mlx5e_set_netdev_dev_addr(netdev);
  
-#ifdef CONFIG_NET_SWITCHDEV

+#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
if (MLX5_CAP_GEN(mdev, vport_group_manager))
netdev->switchdev_ops = _switchdev_ops;
  #endif


Can MLX5_ESWITCH be enabled without NET_SWITCHDEV?


diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c 
b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 66b5fec15313..7d2860252dce 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -806,6 +806,7 @@ void mlx5e_handle_rx_cqe(struct mlx5e_rq *rq, struct 
mlx5_cqe64 *cqe)
   >next.next_wqe_index);
  }
  
+#ifdef CONFIG_MLX5_ESWITCH

  void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
  {
struct net_device *netdev = rq->netdev;
@@ -838,6 +839,7 @@ void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq, struct 
mlx5_cqe64 *cqe)
mlx5_wq_ll_pop(>wq, wqe_counter_be,
   >next.next_wqe_index);
  }
+#endif
  
  static inline void mlx5e_mpwqe_fill_rx_skb(struct mlx5e_rq *rq,

  

Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Jes Sorensen]

On 06/07/2017 07:42 PM, Saeed Mahameed wrote:

Multi-Physical Function Switch (MPFs) is required for when multi-PF
configuration is enabled to allow passing user configured unicast MAC
addresses to the requesting PF.

Before this patch eswitch.c used to manage the HW MPFS l2 table,
eswitch always enabled vport(0) (NIC PF) vport's contexts update on unicast
mac address list changes, to populate the PF's MPFS L2 table accordingly,
even if SRIOV was not enabled.

In downstream patch we would like to allow compiling the driver without
eswitch functionalities, for that we move MPFS l2 table logic out
of eswitch.c into its own file, and provide Kconfig flag (MLX5_MPFS) to
allow compiling out MPFS for those who don't want Multi-PF support

NIC PF netdevice will now directly update MPFS l2 table via the new MPFS
API. VF netdevice has no access to MPFS L2 table, so e-Switch will remain
responsible of updating its PF MPFS l2 table on behalf of its VFs.

Due to this change we also don't require enabling vport(0) (PF vport)
unicast mac changes events anymore, for when SRIOV is not enabled.
Which means eswitch is now activated only on SRIOV activation, and not
required otherwise.



On overall it looks good - one nit.


+static int alloc_l2table_index(struct mlx5_mpfs *l2table, u32 *ix)
+{
+   int err = 0;
+
+   *ix = find_first_zero_bit(l2table->bitmap, l2table->size);
+   if (*ix >= l2table->size)
+   err = -ENOSPC;
+   else
+   __set_bit(*ix, l2table->bitmap);
+
+   return err;
+}


You pass in a pointer for ix but you don't modify it, why not just pass 
in the value?.



+static void free_l2table_index(struct mlx5_mpfs *l2table, u32 ix)
+{
+   __clear_bit(ix, l2table->bitmap);
+}


Here you stick to the u32.

Jes

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 06/07/2017 12:06 AM, Saeed Mahameed wrote:

On Wed, Jun 7, 2017 at 12:46 AM, Jes Sorensen <jsoren...@fb.com> wrote:

Hey Jes,

It is not just about squashing patches, I am working on a series of
patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc
altogether, it will come out cleaner as it will remove all ethernet
sriov/eswitch VF representors and eswitch tc offloads stuff with one
kconfig flag, and yet preserve standard QoS functionality from en_tc.



Saeed,

I realize it is not just about squashing patches, however doing that to
someone else's patches is just broken. The Linux kernel way is to build on
top of patches, if they are valid, rather than throwing them all away and
doing it from scratch again bottom up. If there was something actually wrong
with my patches, and I would love to understand if that is the case, since I
don't know 1/100th of the hardware details that you know, then please share
those details.


Hey Jes,

Sorry for the inconvenience, I am working on a very similar patches,
even before you posted yours.
Your patches are fine, but as i said before, removing eswitch as is
will introduce a small regression in Multi-PF configuration.

the issue is that lately we are having tons of discussions exactly
about this and how to do the driver breakdown
that makes everyone happy, so things are moving relatively slow, but
my work on eswitch is converging.


Gotcha. I deliberately didn't disable eswitch itself in my patch, but 
only the offloading functionality, because of the old discussion 
mentioning that the eswitch needing to be initialized.



Sounds good.


I will post some patches for you to review by end of week.



Could we please start seeing this stuff happen in a public git tree so it is
possible to follow and contribute to the development? It is very frustrating
having to wait for things to appear and and not knowing whether a patch is
integrated or needs to be revised when you have things building on top of
it.


Sure, I will post some patches later today.
I believe they will be fully ready by for submission by End of week.
Again sorry about this.


Awesome!

Jes

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 06/05/2017 05:53 PM, Saeed Mahameed wrote:

On Mon, Jun 5, 2017 at 11:51 PM, Jes Sorensen <jsoren...@fb.com> wrote:

On 06/03/2017 03:37 PM, Or Gerlitz wrote:


On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsoren...@fb.com> wrote:


On 05/28/2017 02:03 AM, Or Gerlitz wrote:



On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.soren...@gmail.com>
wrote:



On 05/27/2017 05:02 PM, Or Gerlitz wrote:




On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen
<jes.soren...@gmail.com>
wrote:




This gets rid of the temporary #ifdef spaghetti and allows the code
to
compile without offload support enabled.





I am pretty sure we can do that exercise you're up to without any
spaghetti cooking and even put more code under that CONFIG directive
(en_rep.c), I'll take that with Saeed.





I want to avoid adding #ifdef CONFIG_foo to the main code in order to
keep
it readable. I did it gradually to make sure I didn't break anything
and
to
allow for it to be bisected in case something did break. If we can move
out
more code from places like en_rep.c into eswitch_offload.c and get it
disabled that way that would be great, but I like to limit the number
of
#ifdefs we add to the actual code.




FWIW (see below), squashing your seven patches to one resulted in a
fairly simple/clear
patch, so if we go that way, no need to have seven commits just for this
piece.




Squashing patches into jumbo patches is inherently broken and bad coding
practice! It makes it way more complicated to debug and bisect in case a
minor detail broke in the process.



Not that pure LOC ##-s is the only/deep measurement, but your overall
changes in the the seven patch series account to:

   5 files changed, 94 insertions(+), 3 deletions(-)

and by no mean this is jumbo or inherently broken and bad coded, so
please slow down please, I looked with care on the resulted patch and
said it's basically ok.



Squashing patches for the sake of squashing patches is inherently broken and
bad. So please calm down and stop this mangling of other peoples' patches.

If you want an alternative, put up a proposal and look at it for comparison
somewhere.


Hey Jes,

It is not just about squashing patches, I am working on a series of
patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc
altogether, it will come out cleaner as it will remove all ethernet
sriov/eswitch VF representors and eswitch tc offloads stuff with one
kconfig flag, and yet preserve standard QoS functionality from en_tc.


Saeed,

I realize it is not just about squashing patches, however doing that to 
someone else's patches is just broken. The Linux kernel way is to build 
on top of patches, if they are valid, rather than throwing them all away 
and doing it from scratch again bottom up. If there was something 
actually wrong with my patches, and I would love to understand if that 
is the case, since I don't know 1/100th of the hardware details that you 
know, then please share those details.



BTW today you can just remove eswitch from driver and non sriov
configuration will perfectly work with no issues.
Even multi PF configuration will also work, but without l2 mac table,
which means PFs can only see packets with their own static (permanent)
mac addresses, user configured macs will not work on Multi PF
configuration.


It sounds like this shakes up things a little and we will have things 
moved to where they actually belong in the hierarchy so that will be a 
good thing in the end :)



For that i will take the l2 table (ConnectX PF mac table) logic out of
eswitch as it is not really an eswitch logic, and move it to core
driver to allow Multi PF configuration to work without eswitch.


Sounds good.


I will post some patches for you to review by end of week.


Could we please start seeing this stuff happen in a public git tree so 
it is possible to follow and contribute to the development? It is very 
frustrating having to wait for things to appear and and not knowing 
whether a patch is integrated or needs to be revised when you have 
things building on top of it.


Jes

[PATCH 1/1] mlx5: Allow TC support to be disabled in the build

[Jes Sorensen]

This provides the option for TC offload support to be disabled in the
driver.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Kconfig   | 10 ++
 drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  4 ++-
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 39 ++-
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c  | 12 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c   | 29 +
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |  1 +
 6 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig 
b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index d91272d437bf..6c2607e2866d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -52,6 +52,16 @@ config MLX5_CORE_EN_ESWITCH_OFFLOADS
 
  If unsure, set to Y
 
+config MLX5_CORE_EN_TC
+   bool "Enable support for TC Filtering Offload Support"
+   default y
+   depends on MLX5_CORE_EN && MLX5_CORE_EN_ESWITCH_OFFLOADS
+   ---help---
+ Say Y here if you want to use Mellanox TC offload support.
+ If set to N, the driver will use the kernel's software implementation.
+
+ If unsure, set to Y
+
 config MLX5_CORE_IPOIB
bool "Mellanox Technologies ConnectX-4 IPoIB offloads support"
depends on MLX5_CORE_EN
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile 
b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 615b33693cc2..72d7d58e9823 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -11,10 +11,12 @@ mlx5_core-$(CONFIG_MLX5_FPGA) += fpga/cmd.o fpga/core.o
 mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o \
en_main.o en_common.o en_fs.o en_ethtool.o en_tx.o \
en_rx.o en_rx_am.o en_txrx.o en_clock.o vxlan.o \
-   en_tc.o en_arfs.o en_rep.o en_fs_ethtool.o en_selftest.o
+   en_arfs.o en_rep.o en_fs_ethtool.o en_selftest.o
 
 mlx5_core-$(CONFIG_MLX5_CORE_EN_DCB) +=  en_dcbnl.o
 
+mlx5_core-$(CONFIG_MLX5_CORE_EN_TC) +=  en_tc.o
+
 mlx5_core-$(CONFIG_MLX5_EN_ESWITCH_OFFLOADS) +=  en_eswitch_offloads.o
 
 mlx5_core-$(CONFIG_MLX5_CORE_IPOIB) += ipoib.o
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c 
b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index cdff04b2aea1..9abdd4189edf 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -2961,38 +2961,10 @@ static int mlx5e_modify_channels_vsd(struct 
mlx5e_channels *chs, bool vsd)
return 0;
 }
 
-static int mlx5e_setup_tc(struct net_device *netdev, u8 tc)
-{
-   struct mlx5e_priv *priv = netdev_priv(netdev);
-   struct mlx5e_channels new_channels = {};
-   int err = 0;
-
-   if (tc && tc != MLX5E_MAX_NUM_TC)
-   return -EINVAL;
-
-   mutex_lock(>state_lock);
-
-   new_channels.params = priv->channels.params;
-   new_channels.params.num_tc = tc ? tc : 1;
-
-   if (!test_bit(MLX5E_STATE_OPENED, >state)) {
-   priv->channels.params = new_channels.params;
-   goto out;
-   }
-
-   err = mlx5e_open_channels(priv, _channels);
-   if (err)
-   goto out;
-
-   mlx5e_switch_priv_channels(priv, _channels, NULL);
-out:
-   mutex_unlock(>state_lock);
-   return err;
-}
-
 static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
  __be16 proto, struct tc_to_netdev *tc)
 {
+#ifdef CONFIG_MLX5_CORE_EN_TC
struct mlx5e_priv *priv = netdev_priv(dev);
 
if (TC_H_MAJ(handle) != TC_H_MAJ(TC_H_INGRESS))
@@ -3019,6 +2991,9 @@ static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 
handle,
tc->mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
 
return mlx5e_setup_tc(dev, tc->mqprio->num_tc);
+#else
+   return -EOPNOTSUPP;
+#endif
 }
 
 static void
@@ -4085,14 +4060,18 @@ static int mlx5e_init_nic_rx(struct mlx5e_priv *priv)
goto err_destroy_direct_tirs;
}
 
+#ifdef CONFIG_MLX5_CORE_EN_TC
err = mlx5e_tc_init(priv);
if (err)
goto err_destroy_flow_steering;
+#endif
 
return 0;
 
+#ifdef CONFIG_MLX5_CORE_EN_TC
 err_destroy_flow_steering:
mlx5e_destroy_flow_steering(priv);
+#endif
 err_destroy_direct_tirs:
mlx5e_destroy_direct_tirs(priv);
 err_destroy_indirect_tirs:
@@ -4106,7 +4085,9 @@ static int mlx5e_init_nic_rx(struct mlx5e_priv *priv)
 
 static void mlx5e_cleanup_nic_rx(struct mlx5e_priv *priv)
 {
+#ifdef CONFIG_MLX5_CORE_EN_TC
mlx5e_tc_cleanup(priv);
+#endif
mlx5e_destroy_flow_steering(priv);
mlx5e_destroy_direct_tirs(priv);
mlx5e_destroy_indirect_tirs(priv);
diff --git a/drivers/net/ethernet/mellanox/m

[PATCH 0/1] Allow TC support to be disabled

[Jes Sorensen]

Hi,

Here is the follow-on patch which allows for TC support to be compiled
out.

It builds on top of my patch set from last week.

Jes


Jes Sorensen (1):
  mlx5: Allow TC support to be disabled in the build

 drivers/net/ethernet/mellanox/mlx5/core/Kconfig   | 10 ++
 drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  4 ++-
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 39 ++-
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c  | 12 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c   | 29 +
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |  1 +
 6 files changed, 65 insertions(+), 30 deletions(-)

-- 
2.13.0

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 06/03/2017 03:37 PM, Or Gerlitz wrote:

On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsoren...@fb.com> wrote:

On 05/28/2017 02:03 AM, Or Gerlitz wrote:


On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.soren...@gmail.com>
wrote:


On 05/27/2017 05:02 PM, Or Gerlitz wrote:



On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.soren...@gmail.com>
wrote:



This gets rid of the temporary #ifdef spaghetti and allows the code to
compile without offload support enabled.




I am pretty sure we can do that exercise you're up to without any
spaghetti cooking and even put more code under that CONFIG directive
(en_rep.c), I'll take that with Saeed.




I want to avoid adding #ifdef CONFIG_foo to the main code in order to
keep
it readable. I did it gradually to make sure I didn't break anything and
to
allow for it to be bisected in case something did break. If we can move
out
more code from places like en_rep.c into eswitch_offload.c and get it
disabled that way that would be great, but I like to limit the number of
#ifdefs we add to the actual code.



FWIW (see below), squashing your seven patches to one resulted in a
fairly simple/clear
patch, so if we go that way, no need to have seven commits just for this
piece.



Squashing patches into jumbo patches is inherently broken and bad coding
practice! It makes it way more complicated to debug and bisect in case a
minor detail broke in the process.


Not that pure LOC ##-s is the only/deep measurement, but your overall
changes in the the seven patch series account to:

  5 files changed, 94 insertions(+), 3 deletions(-)

and by no mean this is jumbo or inherently broken and bad coded, so
please slow down please, I looked with care on the resulted patch and
said it's basically ok.


Squashing patches for the sake of squashing patches is inherently broken 
and bad. So please calm down and stop this mangling of other peoples' 
patches.


If you want an alternative, put up a proposal and look at it for 
comparison somewhere.


Jes

Re: [for-next 4/6] net/mlx5: FPGA, Add basic support for Innova

[Jes Sorensen]

On 05/28/2017 03:24 AM, Ilan Tayari wrote:

-Original Message-
From: Jes Sorensen [mailto:jsoren...@fb.com]

On 05/26/2017 04:29 AM, Saeed Mahameed wrote:

On Thu, May 25, 2017 at 11:48 PM, Jes Sorensen <jsoren...@fb.com> wrote:

On 05/25/2017 06:40 AM, Saeed Mahameed wrote:

Hi Jes,

No, It is clearly stated in the commit message :

"The FPGA is a bump-on-the-wire and thus affects operation of
the mlx5_core driver on the ConnectX ASIC."

Which means mlx5 FPGA user can only write logic which affects only
packets going in/out
A ConnectX chip - so it is only network stuff -.


We have this with other devices in the kernel where a primary device

driver

provides an interface for an additional sub-driver to access another

device

behind it. Like bt-coexist in some of the wifi drivers allowing access

to a

bluetooth device behind it.


Blutooth over wifi or vise versa is a very good example to what you
are requesting.
But, it doesn't fit to what we are trying to do here. mlx5 FGPA is a
ConnectX card feature, not a new protocol.


In that case it would need to be an optional module that can be enabled
or disabled at build time.


Jes,

It is already modular like that. See CONFIG_MLX5_FPGA.


[jes@xpeas netdev.git]$ grep CONFIG_MLX5_FPGA 
drivers/net/ethernet/mellanox/mlx5/core/*

[jes@xpeas netdev.git]$

Which git tree am I supposed to look at?

Jes

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 05/28/2017 02:03 AM, Or Gerlitz wrote:

On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.soren...@gmail.com> wrote:

On 05/27/2017 05:02 PM, Or Gerlitz wrote:


On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.soren...@gmail.com>
wrote:


This gets rid of the temporary #ifdef spaghetti and allows the code to
compile without offload support enabled.



I am pretty sure we can do that exercise you're up to without any
spaghetti cooking and even put more code under that CONFIG directive
(en_rep.c), I'll take that with Saeed.



I want to avoid adding #ifdef CONFIG_foo to the main code in order to keep
it readable. I did it gradually to make sure I didn't break anything and to
allow for it to be bisected in case something did break. If we can move out
more code from places like en_rep.c into eswitch_offload.c and get it
disabled that way that would be great, but I like to limit the number of
#ifdefs we add to the actual code.


FWIW (see below), squashing your seven patches to one resulted in a
fairly simple/clear
patch, so if we go that way, no need to have seven commits just for this piece.


Squashing patches into jumbo patches is inherently broken and bad coding 
practice! It makes it way more complicated to debug and bisect in case a 
minor detail broke in the process.



Just wondering, you are motivated by a wish to put some mlx5
functionalities under their own CONFIG directives which could be
useful when backporting the latest upstream driver into older kernel
and being able not to deal with parts of it, right? in that respect,
are you using SRIOV but not the offloads mode?



The motivation is two-fold, the primary is to be able to disable features
not being used for those who compile a custom kernel and who wish to reduce
the codebase compiled. It also makes it more flexible when back porting the
code to older kernels since it is easier to pick out a smaller subset. I was
going to look into making TC support etc. optional next, but I wanted to
have a discussion about this patchset first.


OKay, I got you.

Re SRIOV, I don't think it would be correct to break the support info few
CONFIG directives. If we want to allow someone to build the driver w.o
SRIOV that's fine, but I don't think we should further go down and disable
some of the SRIOV sub-modes.

Re TC offload support, that's make sense.


OK, so disabling SRIOV and disabling TC makes sense - I'll look at that.

Jes

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 05/27/2017 05:02 PM, Or Gerlitz wrote:

On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.soren...@gmail.com> wrote:

This gets rid of the temporary #ifdef spaghetti and allows the code to
compile without offload support enabled.


Hi Jes,

I am pretty sure we can do that exercise you're up to without any
spaghetti cooking and even put more code under that CONFIG directive
(en_rep.c), I'll take that with Saeed.


Hi Or,

I want to avoid adding #ifdef CONFIG_foo to the main code in order to 
keep it readable. I did it gradually to make sure I didn't break 
anything and to allow for it to be bisected in case something did break. 
If we can move out more code from places like en_rep.c into 
eswitch_offload.c and get it disabled that way that would be great, but 
I like to limit the number of #ifdefs we add to the actual code.



Just wondering, you are motivated by a wish to put some mlx5
functionalities under their own CONFIG directives which could be
useful when backporting the latest upstream driver into older kernel
and being able not to deal with parts of it, right? in that respect,
are you using SRIOV but not the offloads mode?


The motivation is two-fold, the primary is to be able to disable 
features not being used for those who compile a custom kernel and who 
wish to reduce the codebase compiled. It also makes it more flexible 
when back porting the code to older kernels since it is easier to pick 
out a smaller subset. I was going to look into making TC support etc. 
optional next, but I wanted to have a discussion about this patchset first.


Cheers,
Jes

[PATCH 3/7] mlx5: Disable {add,del}_offloaded_rule() code if eswitch offloads are disabled

[Jes Sorensen]

This code will not be called if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is disabled.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h   | 17 +
 .../net/ethernet/mellanox/mlx5/core/eswitch_offloads.c  |  2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 2bf5299..c2b0c02 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -286,6 +286,7 @@ int mlx5_eswitch_get_vport_stats(struct mlx5_eswitch *esw,
 struct mlx5_flow_spec;
 struct mlx5_esw_flow_attr;
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
struct mlx5_flow_spec *spec,
@@ -294,6 +295,22 @@ void
 mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
struct mlx5_flow_handle *rule,
struct mlx5_esw_flow_attr *attr);
+#else
+static inline struct mlx5_flow_handle *
+mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
+   struct mlx5_flow_spec *spec,
+   struct mlx5_esw_flow_attr *attr)
+{
+   return ERR_PTR(-EOPNOTSUPP);
+}
+static inline void
+mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
+   struct mlx5_flow_handle *rule,
+   struct mlx5_esw_flow_attr *attr)
+{
+   return;
+}
+#endif
 
 struct mlx5_flow_handle *
 mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 
tirn);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 8d0af1f..12505ba 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -43,6 +43,7 @@ enum {
FDB_SLOW_PATH
 };
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
struct mlx5_flow_spec *spec,
@@ -122,7 +123,6 @@ mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
esw->offloads.num_flows--;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_set_global_vlan_pop(struct mlx5_eswitch *esw, u8 val)
 {
struct mlx5_eswitch_rep *rep;
-- 
2.9.4

[PATCH 0/7] mlx5: Make eswitch_offloads a compile option

[Jes Sorensen]

Hi,

This changes the code to allow for eswitch_offloads to be compile time
option, reducing the size of the driver module for those who do not
need it.

The patches do it step by step by introducing stubs and then finally
gets rid of all the #ifdefs once the code can compile and link without
including eswitch_offloads.o

Unlike the patches that were discussed on the list back in January,
this does not try to remove eswitch support. It simply allows the
offloads to be disabled.

Cheers,
Jes


Jes Sorensen (7):
  mlx5: Allow support for eswitch offloads to be disabled
  mlx5: eswitch vlan functionality is only called if SRIOV_OFFLOADS is
set
  mlx5: Disable {add,del}_offloaded_rule() code if eswitch offloads are
disabled
  mlx5: Stub out eswitch offload vport functions
  mlx5: Stub out create_vport_rx_rule when eswitch_offloads disabled
  mlx5: Stub out sqs2vport functions
  mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS
is set

 drivers/net/ethernet/mellanox/mlx5/core/Kconfig| 10 +++
 drivers/net/ethernet/mellanox/mlx5/core/Makefile   |  4 +-
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h  | 80 +-
 .../ethernet/mellanox/mlx5/core/eswitch_offloads.c |  1 +
 drivers/net/ethernet/mellanox/mlx5/core/main.c |  2 +-
 5 files changed, 94 insertions(+), 3 deletions(-)

-- 
2.9.4

[PATCH 2/7] mlx5: eswitch vlan functionality is only called if SRIOV_OFFLOADS is set

[Jes Sorensen]

This allows not compiling this code if eswitch offloads are disabled.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h  | 13 +
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index de4e5e8..2bf5299 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -338,10 +338,23 @@ void mlx5_eswitch_unregister_vport_rep(struct 
mlx5_eswitch *esw,
   int vport_index);
 struct net_device *mlx5_eswitch_get_uplink_netdev(struct mlx5_eswitch *esw);
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int mlx5_eswitch_add_vlan_action(struct mlx5_eswitch *esw,
 struct mlx5_esw_flow_attr *attr);
 int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
 struct mlx5_esw_flow_attr *attr);
+#else
+static inline int mlx5_eswitch_add_vlan_action(struct mlx5_eswitch *esw,
+  struct mlx5_esw_flow_attr *attr)
+{
+   return -EOPNOTSUPP;
+}
+static inline int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
+  struct mlx5_esw_flow_attr *attr)
+{
+   return -EOPNOTSUPP;
+}
+#endif
 int __mlx5_eswitch_set_vport_vlan(struct mlx5_eswitch *esw,
  int vport, u16 vlan, u8 qos, u8 set_flags);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index e78dec1..8d0af1f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -122,6 +122,7 @@ mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
esw->offloads.num_flows--;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_set_global_vlan_pop(struct mlx5_eswitch *esw, u8 val)
 {
struct mlx5_eswitch_rep *rep;
@@ -301,6 +302,7 @@ int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
 out:
return err;
 }
+#endif
 
 static struct mlx5_flow_handle *
 mlx5_eswitch_add_send_to_vport_rule(struct mlx5_eswitch *esw, int vport, u32 
sqn)
-- 
2.9.4

[PATCH 4/7] mlx5: Stub out eswitch offload vport functions

[Jes Sorensen]

This code isn't called if CONFIG_MLX5_EN_ESWITCH_OFFLOADS isn't enabled

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h| 20 +++-
 .../ethernet/mellanox/mlx5/core/eswitch_offloads.c   |  2 ++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index c2b0c02..6397384 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -348,6 +348,7 @@ int mlx5_devlink_eswitch_inline_mode_get(struct devlink 
*devlink, u8 *mode);
 int mlx5_eswitch_inline_mode_get(struct mlx5_eswitch *esw, int nvfs, u8 *mode);
 int mlx5_devlink_eswitch_encap_mode_set(struct devlink *devlink, u8 encap);
 int mlx5_devlink_eswitch_encap_mode_get(struct devlink *devlink, u8 *encap);
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void mlx5_eswitch_register_vport_rep(struct mlx5_eswitch *esw,
 int vport_index,
 struct mlx5_eswitch_rep *rep);
@@ -355,12 +356,29 @@ void mlx5_eswitch_unregister_vport_rep(struct 
mlx5_eswitch *esw,
   int vport_index);
 struct net_device *mlx5_eswitch_get_uplink_netdev(struct mlx5_eswitch *esw);
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int mlx5_eswitch_add_vlan_action(struct mlx5_eswitch *esw,
 struct mlx5_esw_flow_attr *attr);
 int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
 struct mlx5_esw_flow_attr *attr);
 #else
+static inline void
+mlx5_eswitch_register_vport_rep(struct mlx5_eswitch *esw, int vport_index,
+   struct mlx5_eswitch_rep *rep)
+{
+   return;
+}
+static inline void
+mlx5_eswitch_unregister_vport_rep(struct mlx5_eswitch *esw, int vport_index)
+{
+   return;
+}
+
+static inline struct net_device *
+mlx5_eswitch_get_uplink_netdev(struct mlx5_eswitch *esw)
+{
+   return NULL;
+}
+
 static inline int mlx5_eswitch_add_vlan_action(struct mlx5_eswitch *esw,
   struct mlx5_esw_flow_attr *attr)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 12505ba..9278a33 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -1126,6 +1126,7 @@ int mlx5_devlink_eswitch_encap_mode_get(struct devlink 
*devlink, u8 *encap)
return 0;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void mlx5_eswitch_register_vport_rep(struct mlx5_eswitch *esw,
 int vport_index,
 struct mlx5_eswitch_rep *__rep)
@@ -1170,3 +1171,4 @@ struct net_device *mlx5_eswitch_get_uplink_netdev(struct 
mlx5_eswitch *esw)
rep = >vport_reps[UPLINK_REP_INDEX];
return rep->netdev;
 }
+#endif
-- 
2.9.4

[PATCH 1/7] mlx5: Allow support for eswitch offloads to be disabled

[Jes Sorensen]

This allows users to disable eswitch offloads. Follow-on patches will
clean up how the eswitch_offloads code is being called and get rid of all
the #ifdefs.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Kconfig| 10 ++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h  | 11 +++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c |  8 
 drivers/net/ethernet/mellanox/mlx5/core/main.c |  2 +-
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig 
b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index 27251a7..27b409e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -32,6 +32,16 @@ config MLX5_CORE_EN_DCB
 
  If unsure, set to Y
 
+config MLX5_CORE_EN_ESWITCH_OFFLOADS
+   bool "Enable support for Mellanox ESwitch Offload Support"
+   default y
+   depends on MLX5_CORE_EN
+   ---help---
+ Say Y here if you want to use Mellanox ESwitch offload support.
+ If set to N, the driver will use the kernel's software implementation.
+
+ If unsure, set to Y
+
 config MLX5_CORE_IPOIB
bool "Mellanox Technologies ConnectX-4 IPoIB offloads support"
depends on MLX5_CORE_EN
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index b746f62..de4e5e8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -243,8 +243,19 @@ struct mlx5_eswitch {
int mode;
 };
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports);
 int esw_offloads_init(struct mlx5_eswitch *esw, int nvports);
+#else
+static inline void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports)
+{
+   return;
+}
+static inline int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
+{
+   return -EOPNOTSUPP;
+}
+#endif
 
 /* E-Switch API */
 int mlx5_eswitch_init(struct mlx5_core_dev *dev);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index f991f66..e78dec1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -393,6 +393,7 @@ int mlx5_eswitch_sqs2vport_start(struct mlx5_eswitch *esw,
return err;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_add_fdb_miss_rule(struct mlx5_eswitch *esw)
 {
struct mlx5_flow_act flow_act = {0};
@@ -425,6 +426,7 @@ static int esw_add_fdb_miss_rule(struct mlx5_eswitch *esw)
kvfree(spec);
return err;
 }
+#endif
 
 #define ESW_OFFLOADS_NUM_GROUPS  4
 
@@ -475,6 +477,7 @@ static void esw_destroy_offloads_fast_fdb_table(struct 
mlx5_eswitch *esw)
 
 #define MAX_PF_SQ 256
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_create_offloads_fdb_tables(struct mlx5_eswitch *esw, int 
nvports)
 {
int inlen = MLX5_ST_SZ_BYTES(create_flow_group_in);
@@ -665,6 +668,7 @@ static void esw_destroy_vport_rx_group(struct mlx5_eswitch 
*esw)
 {
mlx5_destroy_flow_group(esw->offloads.vport_rx_group);
 }
+#endif
 
 struct mlx5_flow_handle *
 mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 
tirn)
@@ -733,6 +737,7 @@ static int esw_offloads_start(struct mlx5_eswitch *esw)
return err;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
 {
struct mlx5_eswitch_rep *rep;
@@ -791,6 +796,7 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
 
return err;
 }
+#endif
 
 static int esw_offloads_stop(struct mlx5_eswitch *esw)
 {
@@ -813,6 +819,7 @@ static int esw_offloads_stop(struct mlx5_eswitch *esw)
return err;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports)
 {
struct mlx5_eswitch_rep *rep;
@@ -829,6 +836,7 @@ void esw_offloads_cleanup(struct mlx5_eswitch *esw, int 
nvports)
esw_destroy_offloads_table(esw);
esw_destroy_offloads_fdb_tables(esw);
 }
+#endif
 
 static int esw_mode_from_devlink(u16 mode, u16 *mlx5_mode)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c 
b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 0c123d5..3d8a41a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1275,7 +1275,7 @@ struct mlx5_core_event_handler {
 };
 
 static const struct devlink_ops mlx5_devlink_ops = {
-#ifdef CONFIG_MLX5_CORE_EN
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
.eswitch_mode_set = mlx5_devlink_eswitch_mode_set,
.eswitch_mode_get = mlx5_devlink_eswitch_mode_get,
   

[PATCH 5/7] mlx5: Stub out create_vport_rx_rule when eswitch_offloads disabled

[Jes Sorensen]

One more vport related function to disable.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h  | 11 ---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 6397384..9a395f3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -295,6 +295,9 @@ void
 mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
struct mlx5_flow_handle *rule,
struct mlx5_esw_flow_attr *attr);
+struct mlx5_flow_handle *
+mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 
tirn);
+
 #else
 static inline struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
@@ -310,11 +313,13 @@ mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
 {
return;
 }
+static inline struct mlx5_flow_handle *
+mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 
tirn)
+{
+   return ERR_PTR(-EOPNOTSUPP);
+}
 #endif
 
-struct mlx5_flow_handle *
-mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 
tirn);
-
 enum {
SET_VLAN_STRIP  = BIT(0),
SET_VLAN_INSERT = BIT(1)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 9278a33..2bda8f5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -670,7 +670,6 @@ static void esw_destroy_vport_rx_group(struct mlx5_eswitch 
*esw)
 {
mlx5_destroy_flow_group(esw->offloads.vport_rx_group);
 }
-#endif
 
 struct mlx5_flow_handle *
 mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 
tirn)
@@ -710,6 +709,7 @@ mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, 
int vport, u32 tirn)
kvfree(spec);
return flow_rule;
 }
+#endif
 
 static int esw_offloads_start(struct mlx5_eswitch *esw)
 {
-- 
2.9.4

[PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

This gets rid of the temporary #ifdef spaghetti and allows the code to
compile without offload support enabled.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Makefile   | 4 +++-
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 9 -
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile 
b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 9e64461..5967b97 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -5,11 +5,13 @@ mlx5_core-y :=main.o cmd.o debugfs.o fw.o eq.o uar.o 
pagealloc.o \
mad.o transobj.o vport.o sriov.o fs_cmd.o fs_core.o \
fs_counters.o rl.o lag.o dev.o
 
-mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o eswitch_offloads.o \
+mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o \
en_main.o en_common.o en_fs.o en_ethtool.o en_tx.o \
en_rx.o en_rx_am.o en_txrx.o en_clock.o vxlan.o \
en_tc.o en_arfs.o en_rep.o en_fs_ethtool.o en_selftest.o
 
 mlx5_core-$(CONFIG_MLX5_CORE_EN_DCB) +=  en_dcbnl.o
 
+mlx5_core-$(CONFIG_MLX5_EN_ESWITCH_OFFLOADS) +=  en_eswitch_offloads.o
+
 mlx5_core-$(CONFIG_MLX5_CORE_IPOIB) += ipoib.o
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 638b84f..320d1c5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -43,7 +43,6 @@ enum {
FDB_SLOW_PATH
 };
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
struct mlx5_flow_spec *spec,
@@ -426,7 +425,6 @@ static int esw_add_fdb_miss_rule(struct mlx5_eswitch *esw)
kvfree(spec);
return err;
 }
-#endif
 
 #define ESW_OFFLOADS_NUM_GROUPS  4
 
@@ -477,7 +475,6 @@ static void esw_destroy_offloads_fast_fdb_table(struct 
mlx5_eswitch *esw)
 
 #define MAX_PF_SQ 256
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_create_offloads_fdb_tables(struct mlx5_eswitch *esw, int 
nvports)
 {
int inlen = MLX5_ST_SZ_BYTES(create_flow_group_in);
@@ -737,7 +734,6 @@ static int esw_offloads_start(struct mlx5_eswitch *esw)
return err;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
 {
struct mlx5_eswitch_rep *rep;
@@ -796,7 +792,6 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
 
return err;
 }
-#endif
 
 static int esw_offloads_stop(struct mlx5_eswitch *esw)
 {
@@ -819,7 +814,6 @@ static int esw_offloads_stop(struct mlx5_eswitch *esw)
return err;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports)
 {
struct mlx5_eswitch_rep *rep;
@@ -836,7 +830,6 @@ void esw_offloads_cleanup(struct mlx5_eswitch *esw, int 
nvports)
esw_destroy_offloads_table(esw);
esw_destroy_offloads_fdb_tables(esw);
 }
-#endif
 
 static int esw_mode_from_devlink(u16 mode, u16 *mlx5_mode)
 {
@@ -1124,7 +1117,6 @@ int mlx5_devlink_eswitch_encap_mode_get(struct devlink 
*devlink, u8 *encap)
return 0;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void mlx5_eswitch_register_vport_rep(struct mlx5_eswitch *esw,
 int vport_index,
 struct mlx5_eswitch_rep *__rep)
@@ -1169,4 +1161,3 @@ struct net_device *mlx5_eswitch_get_uplink_netdev(struct 
mlx5_eswitch *esw)
rep = >vport_reps[UPLINK_REP_INDEX];
return rep->netdev;
 }
-#endif
-- 
2.9.4

[PATCH 6/7] mlx5: Stub out sqs2vport functions

[Jes Sorensen]

These aren't called if CONFIG_MLX5_EN_ESWITCH_OFFLOADS isn't enabled,
so do not build them.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h  | 14 ++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c |  2 --
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 9a395f3..f1a597b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -340,11 +340,25 @@ struct mlx5_esw_flow_attr {
struct mlx5e_tc_flow_parse_attr *parse_attr;
 };
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int mlx5_eswitch_sqs2vport_start(struct mlx5_eswitch *esw,
 struct mlx5_eswitch_rep *rep,
 u16 *sqns_array, int sqns_num);
 void mlx5_eswitch_sqs2vport_stop(struct mlx5_eswitch *esw,
 struct mlx5_eswitch_rep *rep);
+#else
+static inline int mlx5_eswitch_sqs2vport_start(struct mlx5_eswitch *esw,
+  struct mlx5_eswitch_rep *rep,
+  u16 *sqns_array, int sqns_num)
+{
+   return -EOPNOTSUPP;
+}
+static inline void mlx5_eswitch_sqs2vport_stop(struct mlx5_eswitch *esw,
+  struct mlx5_eswitch_rep *rep)
+{
+   return;
+}
+#endif
 
 int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode);
 int mlx5_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 2bda8f5..638b84f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -302,7 +302,6 @@ int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
 out:
return err;
 }
-#endif
 
 static struct mlx5_flow_handle *
 mlx5_eswitch_add_send_to_vport_rule(struct mlx5_eswitch *esw, int vport, u32 
sqn)
@@ -395,7 +394,6 @@ int mlx5_eswitch_sqs2vport_start(struct mlx5_eswitch *esw,
return err;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_add_fdb_miss_rule(struct mlx5_eswitch *esw)
 {
struct mlx5_flow_act flow_act = {0};
-- 
2.9.4

Re: [for-next 4/6] net/mlx5: FPGA, Add basic support for Innova

[Jes Sorensen]

On 05/26/2017 04:29 AM, Saeed Mahameed wrote:

On Thu, May 25, 2017 at 11:48 PM, Jes Sorensen <jsoren...@fb.com> wrote:

On 05/25/2017 06:40 AM, Saeed Mahameed wrote:

Hi Jes,

No, It is clearly stated in the commit message :

"The FPGA is a bump-on-the-wire and thus affects operation of
the mlx5_core driver on the ConnectX ASIC."

Which means mlx5 FPGA user can only write logic which affects only
packets going in/out
A ConnectX chip - so it is only network stuff -.


We have this with other devices in the kernel where a primary device driver
provides an interface for an additional sub-driver to access another device
behind it. Like bt-coexist in some of the wifi drivers allowing access to a
bluetooth device behind it.


Blutooth over wifi or vise versa is a very good example to what you
are requesting.
But, it doesn't fit to what we are trying to do here. mlx5 FGPA is a
ConnectX card feature, not a new protocol.


In that case it would need to be an optional module that can be enabled 
or disabled at build time.


Cheers,
Jes

Re: [for-next 4/6] net/mlx5: FPGA, Add basic support for Innova

[Jes Sorensen]

On 05/25/2017 06:40 AM, Saeed Mahameed wrote:

On Thu, May 25, 2017 at 8:20 AM, Ilan Tayari  wrote:

-Original Message-
Can you put it into different driver? Dumping everything into by far
the biggest nic driver already is already huge headache in terms on
maintainability, debugging and back ports.
Look at how intel splits their drivers.
ixgb, ixgbe, ixgbevf are different drivers thought they have a lot in
common. On one side it's a bit of copy paste, but on the other side


I don't think the ixgb example is the same, simply  ixgb, ixgbe,
ixgbevf have different PCI IDs
and even different SW/FW interfaces. On the other hand, same mlx5
driver can support all of
ConnetX4/5/6 device IDs with the same code flows, same interfaces.


it makes drivers much easier to develop and maintain independently.
ConnectX-6 code and any future hw support doesn't belong to
mlx5 driver at all.


Sorry i must disagree with you on this for the same reasons Ilan mentioned.
We can perfectly achieve the same with modular driver design all under the
same .ko module, with some kconfig flags to reduce the amount of code/features
this .ko provides.


If I get this right, the FPGA is independent and could in theory be used 
for non network stuff. It really should have it's own driver in that 
case, and you should provide accessor functionality via the mlx5 driver.


We have this with other devices in the kernel where a primary device 
driver provides an interface for an additional sub-driver to access 
another device behind it. Like bt-coexist in some of the wifi drivers 
allowing access to a bluetooth device behind it.


Jes

Re: [PATCH v2 00/10] rt2x00: rt2x00: improve calling conventions for register accessors

[Jes Sorensen]

On 05/17/2017 10:46 AM, Arnd Bergmann wrote:

I've managed to split up my long patch into a series of reasonble
steps now.

The first two are required to fix a regression from commit 41977e86c984
("rt2x00: add support for MT7620"), the rest are just cleanups to
have a consistent state across all the register access functions.

  Arnd


Nice work! This is a textbook example of how to do this the right way!

Reviewed-by: Jes Sorensen <jsoren...@fb.com>

Jes

Re: [PATCH] rt2x00: improve calling conventions for register accessors

[Jes Sorensen]

On 05/16/2017 10:19 AM, Arnd Bergmann wrote:

On Tue, May 16, 2017 at 3:44 PM, Jes Sorensen <jes.soren...@gmail.com> wrote:

On 05/16/2017 07:55 AM, Stanislaw Gruszka wrote:


On Mon, May 15, 2017 at 10:39:51AM -0400, David Miller wrote:


Passing return values by reference is and always has been a really
poor way to achieve what these functions are doing.

And frankly, whilst the tool could see what's going on here better, we
should be making code easier rather than more difficult to audit.

I am therefore very much in favor of Arnd's change.

This isn't even a situation where there are multiple return values,
such as needing to signal an error and return an unsigned value at the
same time.

These functions return _one_ value, and therefore they should be
returned as a true return value.



In rt2x00 driver we use poor convention in other kind of registers
accessors like bbp, mac, eeprom. I dislike to changing only rfcsr
accessors and leaving others in the old way. And changing all accessors
would be massive and error prone change, which I'm not prefer either.



That's why you do it in multiple smaller patches rather than one ugly giant
patch.


I did  the first step using a search in vim using

s:\(rt2800_rfcsr_read(.*,.*\), &\(.*\));:\2 = \1);:

but had to introduce a conversion function

static void rt2800_rfcsr_readreg(struct rt2x00_dev *rt2x00dev,
 const unsigned int word, u8 *value)
{
*value = rt2800_rfcsr_read(rt2x00dev, word);
}

to keep the correct types in place for struct rt2x00debug. I now
did all the other ones too, and removed that helper again. The
result in much nicer, but I basically ended up having to do
the same regex search for all of these at once:

static void rt2400pci_bbp_read(struct rt2x00_dev *rt2x00dev,
static void rt2500pci_bbp_read(struct rt2x00_dev *rt2x00dev,
static void rt2500usb_register_read(struct rt2x00_dev *rt2x00dev,
static void rt2500usb_register_read_lock(struct rt2x00_dev *rt2x00dev,
static void rt2500usb_bbp_read(struct rt2x00_dev *rt2x00dev,
static void _rt2500usb_register_read(struct rt2x00_dev *rt2x00dev,
static void rt2800_bbp_read(struct rt2x00_dev *rt2x00dev,
static void rt2800_eeprom_read(struct rt2x00_dev *rt2x00dev,
static void rt2800_rfcsr_readreg(struct rt2x00_dev *rt2x00dev,
static void rt2800_bbp_dcoc_read(struct rt2x00_dev *rt2x00dev,
void (*register_read)(struct rt2x00_dev *rt2x00dev,
void (*register_read_lock)(struct rt2x00_dev *rt2x00dev,
static inline void rt2800_register_read(struct rt2x00_dev *rt2x00dev,
static inline void rt2800_register_read_lock(struct rt2x00_dev *rt2x00dev,
static inline void rt2x00_rf_read(struct rt2x00_dev *rt2x00dev,
static inline void rt2x00_eeprom_read(struct rt2x00_dev *rt2x00dev,
void (*read)(struct rt2x00_dev *rt2x00dev, \
static inline void rt2x00mmio_register_read(struct rt2x00_dev *rt2x00dev,
static inline void _rt2x00_desc_read(__le32 *desc, const u8 word, __le32 *value)
static inline void rt2x00_desc_read(__le32 *desc, const u8 word, u32 *value)
static inline void rt2x00usb_register_read(struct rt2x00_dev *rt2x00dev,
static inline void rt2x00usb_register_read_lock(struct rt2x00_dev *rt2x00dev,
static void rt61pci_bbp_read(struct rt2x00_dev *rt2x00dev,
static void rt73usb_bbp_read(struct rt2x00_dev *rt2x00dev,

and that ended up as a 300KB patch [1]. Splitting it up is clearly possibly,
but I fear that would be more error-prone as we then need to add
those helpers for the other debug stuff as well, and remove it again
afterwards.


True - if the automatic conversion works without automatic intervention, 
I am less worried about it. Personally I would still focus on converting 
one function at a time to reduce the impact of each patch.


Cheers,
Jes

Re: [PATCH] rt2x00: improve calling conventions for register accessors

[Jes Sorensen]

On 05/16/2017 07:55 AM, Stanislaw Gruszka wrote:

On Mon, May 15, 2017 at 10:39:51AM -0400, David Miller wrote:

Passing return values by reference is and always has been a really
poor way to achieve what these functions are doing.

And frankly, whilst the tool could see what's going on here better, we
should be making code easier rather than more difficult to audit.

I am therefore very much in favor of Arnd's change.

This isn't even a situation where there are multiple return values,
such as needing to signal an error and return an unsigned value at the
same time.

These functions return _one_ value, and therefore they should be
returned as a true return value.


In rt2x00 driver we use poor convention in other kind of registers
accessors like bbp, mac, eeprom. I dislike to changing only rfcsr
accessors and leaving others in the old way. And changing all accessors
would be massive and error prone change, which I'm not prefer either.


That's why you do it in multiple smaller patches rather than one ugly 
giant patch.


The rt2x00 current register accessor functions makes the Realtek vendor 
driver equivalent ones look pretty, which is saying something.


Jes

Re: [PATCH] net: wireless: realtek: constify rate_control_ops structures

[Jes Sorensen]

Larry Finger  writes:
> On 12/02/2016 03:50 AM, Bhumika Goyal wrote:
>> The structures rate_control_ops are only passed as an argument to the
>> functions ieee80211_rate_control_{register/unregister}. This argument is
>> of type const, so rate_control_ops having this property can also be
>> declared as const.
>> Done using Coccinelle:
>>
>> @r1 disable optional_qualifier @
>> identifier i;
>> position p;
>> @@
>> static struct rate_control_ops i@p = {...};
>>
>> @ok1@
>> identifier r1.i;
>> position p;
>> @@
>> ieee80211_rate_control_register(@p)
>>
>> @ok2@
>> identifier r1.i;
>> position p;
>> @@
>> ieee80211_rate_control_unregister(@p)
>>
>> @bad@
>> position p!={r1.p,ok1.p,ok2.p};
>> identifier r1.i;
>> @@
>> i@p
>>
>> @depends on !bad disable optional_qualifier@
>> identifier r1.i;
>> @@
>> static
>> +const
>> struct rate_control_ops i={...};
>>
>> @depends on !bad disable optional_qualifier@
>> identifier r1.i;
>> @@
>> +const
>> struct rate_control_ops i;
>>
>> File size before:
>>text data bss dec hex filename
>>1991  104   02095 82f wireless/realtek/rtlwifi/rc.o
>>
>> File size after:
>>text data bss dec hex filename
>>20950   02095 wireless/realtek/rtlwifi/rc.o
>>
[snip]
> The content of your patch is OK; however, your subject is not. By
> convention, "net: wireless: realtek:" is assumed. We do, however,
> include "rtlwifi:" to indicate which part of
> drivers/net/wireless/realtek/ is referenced.

In addition, the first part of the description is useful and the file
size information is reasonable too, but ~20 lines of coccinelle scripts
in the commit message is rather pointless.

Jes

Re: [PATCH] rtl8xxxu: fix tx rate debug output

[Jes Sorensen]

Arnd Bergmann  writes:
> We accidentally print the rate before we know it for txdesc_v2:

Hi Arnd,

Thanks for the patch - Barry Day already posted a patch for this which
Kalle has applied to the wireless tree.

Cheers,
Jes


>
> wireless/realtek/rtl8xxxu/rtl8xxxu_core.c: In function 
> 'rtl8xxxu_fill_txdesc_v2':
> wireless/realtek/rtl8xxxu/rtl8xxxu_core.c:4848:3: error: 'rate' may be used 
> uninitialized in this function [-Werror=maybe-uninitialized]
>
> txdesc_v1 got it right, so let's do it the same way here.
>
> Fixes: b4c3d9cfb607 ("rtl8xxxu: Pass tx_info to fill_txdesc in order to have 
> access to retry count")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 04141e57b8ae..a9137abc3ad9 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -4844,16 +4844,16 @@ rtl8xxxu_fill_txdesc_v2(struct ieee80211_hw *hw, 
> struct ieee80211_hdr *hdr,
>  
>   tx_desc40 = (struct rtl8xxxu_txdesc40 *)tx_desc32;
>  
> - if (rtl8xxxu_debug & RTL8XXXU_DEBUG_TX)
> - dev_info(dev, "%s: TX rate: %d, pkt size %d\n",
> -  __func__, rate, cpu_to_le16(tx_desc40->pkt_size));
> -
>   if (rate_flags & IEEE80211_TX_RC_MCS &&
>   !ieee80211_is_mgmt(hdr->frame_control))
>   rate = tx_info->control.rates[0].idx + DESC_RATE_MCS0;
>   else
>   rate = tx_rate->hw_value;
>  
> + if (rtl8xxxu_debug & RTL8XXXU_DEBUG_TX)
> + dev_info(dev, "%s: TX rate: %d, pkt size %d\n",
> +  __func__, rate, cpu_to_le16(tx_desc40->pkt_size));
> +
>   seq_number = IEEE80211_SEQ_TO_SN(le16_to_cpu(hdr->seq_ctrl));
>  
>   tx_desc40->txdw4 = cpu_to_le32(rate);

Re: [PATCH] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC

[Jes Sorensen]

John Heenan  writes:
> Barry Day has submitted real world reports for the 8192eu and 8192cu.
> This needs to be acknowledged. I have submitted real world reports for
> the 8723bu.

Lets get this a little more clear - first of all, I have asked you to
investigate which part resolves the problem. Rather than 'I randomly
moved something around and it happens to work for me'.

> When it comes down to it, it looks like the kernel code changes are
> really going to be very trivial to fix this problem and we need to
> take the focus off dramatic outbursts over style issues to a strategy
> for getting usable results from real world testing.
>
> Addressing style issues in a dramatic manner to me looks like a mean
> sport for maintainers who line up to easy target first time
> contributors. This mean attitude comes from the top with a well known
> comment about "publicly making fun of people". The polite comments
> over style from Joe Perches and Rafał Miłecki are welcomed.

Once bad code is in place, it is way harder to get rid of it again. It
is *normal* for maintainers to ask contributors to do things
correctly. In addition you have been asked repeatedly by multiple people
to respect coding style, but every patch you posted violated it again in
a different way, instead of spending the little time it would take for
you to get it right.

> An effective strategy would be to insert some printk statements to
> trace what init steps vendor derived drivers do each time
> wpa_supplicant is called and ask real world testers to report their
> results. This is a lot more productive and less error prone than
> laboriously pouring over vendor source code. Alternative drivers that
> use vendor code from Realtek is enormously complicated and a huge pain
> to make sense of.
>
> Joe Sorensen's driver code is far easier to make sense of and it is a
> shame Realtek don't come to the party. Joe Sorensens's code take takes
> advantage of the excellent work of kernel contributors to the mac80211
> driver.

Now you are pissing on my name - do you really want to be taken
seriously here?

> Previous comments I made about enable_rf, rtl8xxxu_start,
> rtl8xxxu_init_device etc should be clarified. I will leave it for the
> moment as it currently serves no direct useful purpose.

I have made it very clear I want this issue resolved, but I want it
done right.

Jes

Re: [PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower

[Jes Sorensen]

Dave Jones <s.dave.jo...@gmail.com> writes:
> On Fri, Nov 04, 2016 at 09:56:00AM -0400, Jes Sorensen wrote:
>>
>> Joe Perches <j...@perches.com> writes:
>> > On Sun, 2016-10-30 at 19:02 -0400, Jes Sorensen wrote:
>> >> Code is 80 characters wide, and comments are /* */ never the ugly C++
>> >> crap.
>> >
>> > You might look at the recent Linus Torvalds authored commit
>> > 5e467652ffef (?printk: re-organize log_output() to be more legible")
>> > which does both of those: c99 // comments and > 80 columns.
>> >
>> > Absolutes are for zealots.
>>
>> What Linus does in his code, is totally up to him. What I pull into the
>> driver that *I* maintain, is up to me. It is perfectly normal to expect
>> submitters to respect the coding style of the piece of code they are
>> trying to edit.
>
> Bullshit.  It's perfectly normal to respect Linux coding style described in
> Documentation/CodingStyle.  Now let's back to the topic, could you
> apply John's patch or you just wanna improve your driver is 100% bug free?

First of all, I call for proper CodingStyle to be applied to my driver,
and I expect someone posting a patch to respect the codingstyle of the
driver in question. It is simple respect for the code. If you consider
that BS - that is on you!

Second I am NOT applying that patch as I have stated repeatedly because
I am not convinced it is safe to do so and it changes the code flow for
one type of chip and not the rest. In addition it uses a broken approach
to doing chip specific changes.

In short, the patch is broken!

Jes

Re: [PATCH] net: alteon: acenic: use new api ethtool_{get|set}_link_ksettings

[Jes Sorensen]

On 11/05/16 11:17, Philippe Reynes wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
> Signed-off-by: Philippe Reynes <trem...@gmail.com>
> ---
>  drivers/net/ethernet/alteon/acenic.c |   65 ++---
>  1 files changed, 35 insertions(+), 30 deletions(-)

Nothing that sticks out to me

Acked-by: Jes Sorensen <jes.soren...@gmail.com>

Jes


> diff --git a/drivers/net/ethernet/alteon/acenic.c 
> b/drivers/net/ethernet/alteon/acenic.c
> index a5c1e29..16f0c70 100644
> --- a/drivers/net/ethernet/alteon/acenic.c
> +++ b/drivers/net/ethernet/alteon/acenic.c
> @@ -429,14 +429,16 @@
>"acenic.c: v0.92 08/05/2002  Jes Sorensen, linux-ace...@sunsite.dk\n"
>"http://home.cern.ch/~jes/gige/acenic.html\n;;
>  
> -static int ace_get_settings(struct net_device *, struct ethtool_cmd *);
> -static int ace_set_settings(struct net_device *, struct ethtool_cmd *);
> +static int ace_get_link_ksettings(struct net_device *,
> +   struct ethtool_link_ksettings *);
> +static int ace_set_link_ksettings(struct net_device *,
> +   const struct ethtool_link_ksettings *);
>  static void ace_get_drvinfo(struct net_device *, struct ethtool_drvinfo *);
>  
>  static const struct ethtool_ops ace_ethtool_ops = {
> - .get_settings = ace_get_settings,
> - .set_settings = ace_set_settings,
>   .get_drvinfo = ace_get_drvinfo,
> + .get_link_ksettings = ace_get_link_ksettings,
> + .set_link_ksettings = ace_set_link_ksettings,
>  };
>  
>  static void ace_watchdog(struct net_device *dev);
> @@ -2579,43 +2581,44 @@ static int ace_change_mtu(struct net_device *dev, int 
> new_mtu)
>   return 0;
>  }
>  
> -static int ace_get_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
> +static int ace_get_link_ksettings(struct net_device *dev,
> +   struct ethtool_link_ksettings *cmd)
>  {
>   struct ace_private *ap = netdev_priv(dev);
>   struct ace_regs __iomem *regs = ap->regs;
>   u32 link;
> + u32 supported;
>  
> - memset(ecmd, 0, sizeof(struct ethtool_cmd));
> - ecmd->supported =
> - (SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full |
> -  SUPPORTED_100baseT_Half | SUPPORTED_100baseT_Full |
> -  SUPPORTED_1000baseT_Half | SUPPORTED_1000baseT_Full |
> -  SUPPORTED_Autoneg | SUPPORTED_FIBRE);
> + memset(cmd, 0, sizeof(struct ethtool_link_ksettings));
>  
> - ecmd->port = PORT_FIBRE;
> - ecmd->transceiver = XCVR_INTERNAL;
> + supported = (SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full |
> +  SUPPORTED_100baseT_Half | SUPPORTED_100baseT_Full |
> +  SUPPORTED_1000baseT_Half | SUPPORTED_1000baseT_Full |
> +  SUPPORTED_Autoneg | SUPPORTED_FIBRE);
> +
> + cmd->base.port = PORT_FIBRE;
>  
>   link = readl(>GigLnkState);
> - if (link & LNK_1000MB)
> - ethtool_cmd_speed_set(ecmd, SPEED_1000);
> - else {
> + if (link & LNK_1000MB) {
> + cmd->base.speed = SPEED_1000;
> + } else {
>   link = readl(>FastLnkState);
>   if (link & LNK_100MB)
> - ethtool_cmd_speed_set(ecmd, SPEED_100);
> + cmd->base.speed = SPEED_100;
>   else if (link & LNK_10MB)
> - ethtool_cmd_speed_set(ecmd, SPEED_10);
> + cmd->base.speed = SPEED_10;
>   else
> - ethtool_cmd_speed_set(ecmd, 0);
> + cmd->base.speed = 0;
>   }
>   if (link & LNK_FULL_DUPLEX)
> - ecmd->duplex = DUPLEX_FULL;
> + cmd->base.duplex = DUPLEX_FULL;
>   else
> - ecmd->duplex = DUPLEX_HALF;
> + cmd->base.duplex = DUPLEX_HALF;
>  
>   if (link & LNK_NEGOTIATE)
> - ecmd->autoneg = AUTONEG_ENABLE;
> + cmd->base.autoneg = AUTONEG_ENABLE;
>   else
> - ecmd->autoneg = AUTONEG_DISABLE;
> + cmd->base.autoneg = AUTONEG_DISABLE;
>  
>  #if 0
>   /*
> @@ -2626,13 +2629,15 @@ static int ace_get_settings(struct net_device *dev, 
> struct ethtool_cmd *ecmd)
>   ecmd->txcoal = readl(>TuneTxCoalTicks);
>   ecmd->rxcoal = readl(>TuneRxCoalTicks);
>  #endif
> - ecmd->maxtxpkt = readl(>TuneMaxTxDesc);
> - ecmd->maxrxpkt = readl(>TuneMaxRxDesc);
> +
> + ethtool_convert_legac

Re: [PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower

[Jes Sorensen]

Joe Perches <j...@perches.com> writes:
> On Sun, 2016-10-30 at 19:02 -0400, Jes Sorensen wrote:
>> Code is 80 characters wide, and comments are /* */ never the ugly C++
>> crap.
>
> You might look at the recent Linus Torvalds authored commit
> 5e467652ffef (?printk: re-organize log_output() to be more legible")
> which does both of those: c99 // comments and > 80 columns.
>
> Absolutes are for zealots.

What Linus does in his code, is totally up to him. What I pull into the
driver that *I* maintain, is up to me. It is perfectly normal to expect
submitters to respect the coding style of the piece of code they are
trying to edit.

Jes

Re: [PATCH] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC

[Jes Sorensen]

John Heenan  writes:
> The rtl8723bu wireless IC shows evidence of a more agressive approach to
> power saving, powering down its RF side when there is no wireless
> interfacing but leaving USB interfacing intact. This makes the wireless
> IC more suitable for use in devices which need to keep their power use
> as low as practical, such as tablets and Surface Pro type devices.
>
> In effect this means that a full initialisation must be performed
> whenever a wireless interface is brought up. It also means that
> interpretations of power status from general wireless registers should
> not be relied on to influence an init sequence.
>
> The patch works by forcing a fuller initialisation and forcing it to
> occur more often in code paths (such as occurs during a low level
> authentication that initiates wireless interfacing).
>
> The initialisation sequence is now more consistent with code based
> directly on vendor code. For example while the vendor derived code
> interprets a register as indcating a particular powered state, it does
> not use this information to influence its init sequence.
>
> The rtl8723bu device has a unique USB VID and PID. This is taken
> advantage of for the patch to ensure only rtl8723bu devices are affected
> by this patch.
>
> With this patch wpa_supplicant reliably and consistently connects with
> an AP. Before a workaround such as executing rmmod and modprobe before
> each call to wpa_supplicant worked with some distributions.
>
> Signed-off-by: John Heenan 
> ---
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c  | 24 
> ++
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 04141e5..f36e674 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -79,6 +79,8 @@ MODULE_PARM_DESC(dma_agg_pages, "Set DMA aggregation pages 
> (range 1-127, 0 to di
>  #define RTL8XXXU_TX_URB_LOW_WATER25
>  #define RTL8XXXU_TX_URB_HIGH_WATER   32
>  
> +#define USB_PRODUCT_ID_RTL8723BU 0xb720
> +

Absolutely not! You have no guarantee that this is the only id used for
8723bu devices, and adding a #define for each is not going to happen.

>  static int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
> struct rtl8xxxu_rx_urb *rx_urb);
>  
> @@ -3892,6 +3894,7 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
>   u8 val8;
>   u16 val16;
>   u32 val32;
> +  struct usb_device_descriptor *udesc = >udev->descriptor;

Indentaiton
  
>   /* Check if MAC is already powered on */
>   val8 = rtl8xxxu_read8(priv, REG_CR);
> @@ -3900,7 +3903,9 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
>* Fix 92DU-VC S3 hang with the reason is that secondary mac is not
>* initialized. First MAC returns 0xea, second MAC returns 0x00
>*/
> - if (val8 == 0xea)
> + if (val8 == 0xea
> + || (udesc->idVendor == USB_VENDOR_ID_REALTEK
> + &&  udesc->idProduct == USB_PRODUCT_ID_RTL8723BU))
>   macpower = false;
>   else
>   macpower = true;

Please respect proper kernel coding style!

> @@ -5776,9 +5781,17 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>   struct rtl8xxxu_tx_urb *tx_urb;
>   unsigned long flags;
>   int ret, i;
> + struct usb_device_descriptor *udesc = >udev->descriptor;
>  
>   ret = 0;
>  
> + if(udesc->idVendor == USB_VENDOR_ID_REALTEK
> + && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU) {
> + ret = rtl8xxxu_init_device(hw);
> + if (ret)
> + goto error_out;
> + }
> +

As mentioned previously, if this is to be changed here, it has to be
matched in the _stop section too. It also has to be investigated exactly
why this matters for 8723bu. It is possible this matters for other
devices, but we need to find out exactly what causes this not moving a
major block of init code around like this.

>   init_usb_anchor(>rx_anchor);
>   init_usb_anchor(>tx_anchor);
>   init_usb_anchor(>int_anchor);
> @@ -6080,9 +6093,12 @@ static int rtl8xxxu_probe(struct usb_interface 
> *interface,
>   goto exit;
>   }
>  
> - ret = rtl8xxxu_init_device(hw);
> - if (ret)
> + if(!(id->idVendor == USB_VENDOR_ID_REALTEK
> + && id->idProduct == USB_PRODUCT_ID_RTL8723BU)) {
> + ret = rtl8xxxu_init_device(hw);
> + if (ret)
>   goto exit;
> + }

Again, this coding style abuse will never go into this driver,

Jes

Re: [PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower

[Jes Sorensen]

John Heenan  writes:
> Thanks for your reply.
>
> The code was tested on a Cube i9 which has an internal rtl8723bu.
>
> No other devices were tested.
>
> I am happy to accept in an ideal context hard coding macpower is
> undesirable, the comment is undesirable and it is wrong to assume the
> issue is not unique to the rtl8723bu.
>
> Your reply is idealistic. What can I do now?  I should of course have
> factored out other untested devices in my patches. The apparent
> concern you have with process over outcome is a useful lesson.
>
> We are not in an ideal situation. The comment is of course relevant
> and useful to starting a process to fixing a real bug I do not have
> sufficient information to refine any further for and others do. In the
> circumstances nothing really more can be expected.

Well you should start by reporting the issue and either providing a
patch that only affects 8723bu, or work on a generic solution. I
appreciate patches, but I do not appreciate patches that will make
something work for one person and break for everyone else - I spent a
lot of time making sure the driver works across the different devices.

The comment violates all Linux standards - first rule when modifying
code is to respect the style of the code you are dealing with.

Code is 80 characters wide, and comments are /* */ never the ugly C++
crap.

> My patch cover letter, [PATCH 0/2] provides evidence of a mess with
> regard to determining macpower for the rtl8723bu and what is
> subsequently required. This is important.
>
> The kernel driver code is very poorly documented and there is not a
> single source reference to device documentation. For example macpower
> is noting more than a setting that is true or false according to
> whether a read of a particular register return 0xef or not. Such value
> was never obtained so a full init sequence was never performed.

The kernel driver is documented with the information I have - there is
NO device documentation because Realtek refuses to provide any. I have
written the driver based on what I have retried by reading the vendor
drivers. If you can provide better documentation, I certainly would love
to get it.

> It would be helpful if you could provide a link to device references.
> As it is, how am I supposed to revise the patch without relevant
> information?

Look at the USB device table, it shows you which devices are supported.

> My patch code works with the Cube i9, as is, despite a lack of
> adequate information. Before it did not. That is a powerful statement

The driver works with a lot of different devices in itself that is a
powerful statement!

Yes I want to see it work with as many devices as possible, but just
moving things around without balancing it and not explaining why is not
a fix. If we move more of the init sequence to _start() you also have to
move matching pieces to _stop().

Jes

Re: [PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower

[Jes Sorensen]

John Heenan  writes:
> Code tests show data returned by rtl8xxxu_read8(priv, REG_CR), used to set
> macpower, is never 0xea. It is only ever 0x01 (first time after modprobe)
> using wpa_supplicant and 0x00 thereafter using wpa_supplicant. These results
> occurs with 'Fix for authentication failure' [PATCH 1/2] in place.
>
> Whatever was returned, code tests always showed that at least
> rtl8xxxu_init_queue_reserved_page(priv);
> is always required. Not called if macpower set to true.
>
> Please see cover letter, [PATCH 0/2], for more information from tests.
>
> For rtl8xxxu-devel branch of 
> git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
>
> Signed-off-by: John Heenan 
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index f25b4df..aae05f3 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -3904,6 +3904,7 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
>   macpower = false;
>   else
>   macpower = true;
> + macpower = false; // Code testing shows macpower must always be set to 
> false to avoid failure
>  
>   ret = fops->power_on(priv);
>   if (ret < 0) {

Sorry but this patch is neither serious nor acceptable. First of all,
hardcoding macpower like this right after an if statement is plain
wrong, second your comments violate all kernel rules.

Second, you argue this was tested using code test - on which device? Did
you test it on all rtl8xxxu based devices or just rtl8723bu?

NACK

Jes

Re: [PATCH] realtek: rtl8xxxu: Use const init arrays

[Jes Sorensen]

Joe Perches <j...@perches.com> writes:
> On Sat, 2016-10-01 at 16:32 -0400, Jes Sorensen wrote:
>> Your output shows it moving to the text segment - if it's in a different
>> segment, eg. rodata, you should use output demonstrating that to justify
>> the change.
>
> For size, rodata _is_ text

Well then maybe use something which provides accurate data.

Jes

Re: [PATCH] realtek: rtl8xxxu: Use const init arrays

[Jes Sorensen]

Joe Perches <j...@perches.com> writes:
> On Sat, 2016-10-01 at 14:53 -0400, Jes Sorensen wrote:
>> Joe Perches <j...@perches.com> writes:
>> > Make the init arrays const to reduce data.
>> > $ size drivers/net/wireless/realtek/rtl8xxxu/built-in.o*
>> > (allyesconfig: x86-32)
>> >    text   data bss dec hex filename
>> >   80107 13651 58 93816 16e78
>> > drivers/net/wireless/realtek/rtl8xxxu/built-in.o.new
>> >   65303 28435 58 93796 16e64
>> > drivers/net/wireless/realtek/rtl8xxxu/built-in.o.old
>> In total you grow the kernel by 20 bytes. You reduce the data segment
>> substantially while growing the text segment instead.
>
> No, not really.   The alignment boundaries move a bit for
> this particular compilation.  It could go the other way for
> a different compiler version or set of CONFIG options.
>
> What's important is multiple pages of .data move to .rodata.

Your output shows it moving to the text segment - if it's in a different
segment, eg. rodata, you should use output demonstrating that to justify
the change.

Jes

Re: [PATCH] realtek: rtl8xxxu: Use const init arrays

[Jes Sorensen]

Joe Perches  writes:
> Make the init arrays const to reduce data.
>
> $ size drivers/net/wireless/realtek/rtl8xxxu/built-in.o* (allyesconfig: 
> x86-32)
>text  data bss dec hex filename
>   80107 13651  58   93816   16e78 
> drivers/net/wireless/realtek/rtl8xxxu/built-in.o.new
>   65303 28435  58   93796   16e64 
> drivers/net/wireless/realtek/rtl8xxxu/built-in.o.old
>
> Signed-off-by: Joe Perches 

In total you grow the kernel by 20 bytes. You reduce the data segment
substantially while growing the text segment instead.

If any architecture replicates the text segment onto individual numa
nodes, this would actually be a real loss rather than a win. Some archs
used to do this, not sure if they are doing it anymore.

I am not against this patch, but I am not sure it's really a win either.

Jes

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Jes Sorensen]

Joe Perches  writes:
> On Sat, 2016-09-24 at 14:06 -0500, Larry Finger wrote:
>> On 09/24/2016 12:32 PM, Joe Perches wrote:
> []
>> o Reindent all the switch/case blocks to a more normal
>>   kernel style (git diff -w would show no changes here)
>> That sounds like busy work to me, but if you want to do it, go ahead.
>
> It's really just to make the comparison case block reductions
> easier to verify for later steps done
>
>> > o cast, spacing and parenthesis reductions
>> >   Lots of odd and somewhat unique styles in various
>> >   drivers, looks like too many individual authors without
>> >   a style guide / code enforcer using slightly different
>> >   personalized code.  Glancing at the code, it looks to be
>> >   similar logic, just written in different styles.
>> Same comment.
>
> Same rationale
>
>> > o Logic changes like
>> >   from:
>> > if (foo) func(..., bar, ...); else func(..., baz, ...);
>> >   to:
>> > func(..., foo ? bar : baz, ...);
>> >   to make the case statement code blocks more consistent
>> >   and emit somewhat smaller object code.
>> I find if .. else constructs much easier to read than the cond ?  :  
>> form. I would reject any such patches.
>
>  I think object code reduction generally a good thing
> but then again, I'm not a maintainer here.

I missed this part, but I am with Larry here - 'foo ? bar : boo' are
just obfuscating the code and far less clear than if or switch
statements.

Jes

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

[Jes Sorensen]

Larry Finger  writes:
> On 09/24/2016 12:32 PM, Joe Perches wrote:
>> Is there any value in that or is Jes' work going to make
>> doing any or all of this unnecessary and futile?
>
> That is not yet determined. The only driver that is to be replaced at
> this point is rtl8192cu. Jes only has USB I/O for his driver. We are
> looking at adding SDIO, and once that is done, PCI should be possible.

If someone else wants to address PCI then it could happen quite soon,
but at the current schedule I don't see PCI happen in my driver for at
least a year, probably more.

If you can reduce the size of rtlwifi in the mean time that probably
isn't going to upset a lot of people.

Jes

Re: rtl8xxxu: fix spelling mistake "firmare" -> "firmware"

[Jes Sorensen]

Kalle Valo  writes:
> Colin Ian King  wrote:
>> From: Colin Ian King 
>> 
>> Trivial fix to spelling mistakes in dev_dbg message.
>> 
>> Signed-off-by: Colin Ian King 
>> Reviewed-by: Julian Calaby 
>
> I assume Jes will take this.

It's on my list, sorry been swamped in LPC stuff and other work the last
couple of days.

Cheers,
Jes

Re: fix:rtl8xxxu_core: mark symbols static where possible

[Jes Sorensen]

Kalle Valo  writes:
> Baoyou Xie  wrote:
>> We get 1 warning about global functions without a declaration
>> in the rtl8xxxu rtl8xxxu_core.c when building with W=1:
>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c:898:1:
>> warning: no previous prototype for 'rtl8xxxu_gen1_h2c_cmd'
>> [-Wmissing-prototypes]
>> 
>> In fact, this function is only used in the file in which it is declared
>> and don't need a declaration, but can be made static.
>> so this patch marks it 'static'.
>> 
>> Signed-off-by: Baoyou Xie 
>
> The title should be "rtl8xxxu: ". See:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#subject_name
>
> Also I assume Jes will take this.

Yes to both accounts!

Thanks,
Jes

Re: [PATCH 2/3] staging/rtl8192e: use s8 instead of char

[Jes Sorensen]

Stefan Lippers-Hollmann <s@gmx.de> writes:
> Hi
>
> On 2016-07-20, Arnd Bergmann wrote:
>> On Wednesday, July 20, 2016 11:33:43 AM CEST Jes Sorensen wrote:
>> > Arnd Bergmann <a...@arndb.de> writes:  
>> > > On Wednesday, July 20, 2016 7:25:19 AM CEST Jes Sorensen wrote:  
>> > >> Arnd Bergmann <a...@arndb.de> writes:
> [...]
>> Yes, I was just agreeing here that it's not worth doing that one.
>> As far as I can see, the evolution of these devices is
>> 
>> RTL81xxU (2008)
>> RTL81xxSU (2009)
>> RTL81xxCU (2010)
>
> There is also RTL81xxDU, apparently from 2011, a dualband device
> coming in several variants (single MAC + single PHY, double MAC +
> double PHY and double PHY); e.g. 0bda:8194 (single PHY + single MAC).
>
> While probably not overly common, it was/ is (hardware-wise) a pretty
> interesting device due to its support for 5 GHz[1] - actually I hoped
> it to be a (supported-) RTL8192CU variant when I bought it. 
> Unfortunately no driver[2] made it to staging or the proper kernel.

I actually have one of those in my USB dongle box, but as you say, not
overly common so not sure if/when I'll get to it.

Adding 8192du support for 2.4GHz to rtl8xxxu probably wouldn't be too
complicated.

Cheers,
Jes

Re: [PATCH 3/3] staging/rtl8192e: avoid comparing unsigned type >= 0

[Jes Sorensen]

Arnd Bergmann <a...@arndb.de> writes:
> There is one remaining warning about a type limit check in rtl8192e:
>
> staging/rtl8192e/rtl819x_TSProc.c:326:14: error: comparison is always true 
> due to limited range of data type [-Werror=type-limits]
>
> This changes a macro into a local function to clarify the types and simplify
> the check while removing the warning.
>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---
>  drivers/staging/rtl8192e/rtl819x_Qos.h| 3 ---
>  drivers/staging/rtl8192e/rtl819x_TSProc.c | 5 +
>  2 files changed, 5 insertions(+), 3 deletions(-)

Looks good to me!

Acked-by: Jes Sorensen <jes.soren...@redhat.com>

> diff --git a/drivers/staging/rtl8192e/rtl819x_Qos.h 
> b/drivers/staging/rtl8192e/rtl819x_Qos.h
> index 463122db6d29..61da8f7475bb 100644
> --- a/drivers/staging/rtl8192e/rtl819x_Qos.h
> +++ b/drivers/staging/rtl8192e/rtl819x_Qos.h
> @@ -169,9 +169,6 @@ union qos_tclas {
>   } TYPE2_8021Q;
>  };
>  
> -#define IsACValid(ac)((ac >= 0 && ac <= 7) ? true : false)
> -
> -
>  union aci_aifsn {
>   u8  charData;
>  
> diff --git a/drivers/staging/rtl8192e/rtl819x_TSProc.c 
> b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> index 2c8a526773ed..a966a8e490ab 100644
> --- a/drivers/staging/rtl8192e/rtl819x_TSProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> @@ -306,6 +306,11 @@ static void MakeTSEntry(struct ts_common_info 
> *pTsCommonInfo, u8 *Addr,
>   pTsCommonInfo->TClasNum = TCLAS_Num;
>  }
>  
> +static bool IsACValid(unsigned int tid)
> +{
> + return tid < 7;
> +}
> +
>  bool GetTs(struct rtllib_device *ieee, struct ts_common_info **ppTS,
>  u8 *Addr, u8 TID, enum tr_select TxRxSelect, bool bAddNewTs)
>  {

Re: [PATCH 2/3] staging/rtl8192e: use s8 instead of char

[Jes Sorensen]

Arnd Bergmann <a...@arndb.de> writes:
> On Wednesday, July 20, 2016 7:25:19 AM CEST Jes Sorensen wrote:
>> Arnd Bergmann <a...@arndb.de> writes:
>> Well it really all depends on how much time I have and how much others
>> step up and help contribute to the code. For rtl8xxxu my plans are as
>> follows:
>> 
>> 1) rtl8188eu support, since this is the most widely distributed USB
>> dongle which isn't currently supported by a non staging driver. I am
>> currently working on this together with Andrea Merello.
>
> Ok, cool.
>
>> 2) Beacon support for IBSS and AP mode - hopefully this should make it
>> possible to default rtl8xxxu for rtl8192cu/rtl8188cu devices and disable
>> them in rtlwifi.
>
> Do we have any indication that those two actually work in rtlwifi at the
> moment? My experience seems to match the recommendations for all the
> raspberry pi users that use yet another (worse looking) driver:
>
> https://github.com/raspberrypi/linux/commit/9ee31007a5032a3afe2fcb20c36b34f0ad57df56

I am not really authoritative on that one. I tried it in station mode
and it didn't work well for me. I never played with AP mode - It may
work better in IBSS or AP mode than it does in station mode. I don't
like to pull the rug away under people, which is why I haven't pushed
for this.

>> > As one data point that I can provide (but you are probably
>> > aware of), I could never get my rtl8188cus stick to work with
>> > rtlwifi, but I found the older r8712u device to work fine with
>> > the staging/rtl8712 driver.
>> 
>> I'd love to hear if the rtl8188cus works better with rtl8xxxu.
>
> It took me far too long to get the driver running on my machine (all my 
> fault),
> but I've tested it now. Unfortunately there is something very wrong
> with my home wireless network at the moment, so I can only confirm
> that it doesn't work any worse than my Intel Wireless card on 2.4GHz,
> but that isn't any good (5GHz devices are fine, but that doesn't
> help on a 2.4GHz-only device).
>
> This is what I see in the kernel log
>
> [  773.862848] usb 2-1.2: new high-speed USB device number 8 using ehci-pci
> [  773.957415] usb 2-1.2: New USB device found, idVendor=0bda, idProduct=8176
> [ 773.957425] usb 2-1.2: New USB device strings: Mfr=1, Product=2,
> SerialNumber=3
> [  773.957430] usb 2-1.2: Manufacturer: Realtek
> [  773.957433] usb 2-1.2: SerialNumber: 00e04c01
> [  774.115182] usb 2-1.2: Vendor: Realtek
> [  774.115192] usb 2-1.2: Product: 
> [  774.115199] usb 2-1.2: rtl8192cu_parse_efuse: dumping efuse (0x80 bytes):
> [  774.115206] usb 2-1.2: 00: 29 81 00 74 ed 00 00 00
> [  774.115212] usb 2-1.2: 08: ff 00 da 0b 76 81 01 41
> [  774.115219] usb 2-1.2: 10: 32 00 85 62 7e ad 5c f3
> [  774.115225] usb 2-1.2: 18: 70 15 9c b1 0a 03 52 65
> [  774.115231] usb 2-1.2: 20: 61 6c 74 65 6b 00 02 03
> [  774.115237] usb 2-1.2: 28: 00 00 00 00 00 00 00 00
> [  774.115242] usb 2-1.2: 30: 00 00 00 00 00 00 00 00
> [  774.115248] usb 2-1.2: 38: 00 00 00 00 00 00 00 00
> [  774.115254] usb 2-1.2: 40: 00 00 00 00 00 00 00 00
> [  774.115260] usb 2-1.2: 48: 00 00 00 00 00 00 00 00
> [  774.115265] usb 2-1.2: 50: 00 00 00 00 00 00 00 00
> [  774.115271] usb 2-1.2: 58: 06 00 2a 2a 2a 00 00 00
> [  774.115277] usb 2-1.2: 60: 2a 2a 2a 00 00 00 00 00
> [  774.115283] usb 2-1.2: 68: 00 00 00 00 04 04 04 00
> [  774.115289] usb 2-1.2: 70: 00 00 00 00 00 00 05 00
> [  774.115295] usb 2-1.2: 78: 10 00 00 00 36 00 00 00
> [ 774.115302] usb 2-1.2: RTL8188CU rev A (TSMC) 1T1R, TX queues 2,
> WiFi=1, BT=0, GPS=0, HI PA=0
> [  774.115308] usb 2-1.2: RTL8188CU MAC: 5c:f3:70:15:9c:b1
> [ 774.115314] usb 2-1.2: rtl8xxxu: Loading firmware
> rtlwifi/rtl8192cufw_TMSC.bin
> [  774.115409] usb 2-1.2: Firmware revision 80.0 (signature 0x88c1)
> [  775.692344] rtl8xxxu 2-1.2:1.0 wlan1: renamed from wlan0
> [  775.721151] IPv6: ADDRCONF(NETDEV_UP): wlan1: link is not ready
> [  775.746653] IPv6: ADDRCONF(NETDEV_UP): wlan1: link is not ready
> [  775.798780] IPv6: ADDRCONF(NETDEV_UP): wlan1: link is not ready
> [  788.414618] wlan2: authenticate with 22:4e:7f:6f:5b:3c
> [  788.452485] wlan2: send auth to 22:4e:7f:6f:5b:3c (try 1/3)
> [  788.457926] wlan2: authenticated
> [  788.462261] wlan2: associate with 22:4e:7f:6f:5b:3c (try 1/3)
> [ 788.475159] wlan2: RX AssocResp from 22:4e:7f:6f:5b:3c (capab=0x431
> status=0 aid=1)
> [  788.504683] wlan2: associated

That all looks reasonable to me.

> throughput for me is 2mbit/s, compared to my intel 2x2 wireless that gets
> 5mbit/s on the same network, but I guess that doesn't really mean much
> as long as I have problems with the infrastructure.

:) Note the rtl8xxxu driver doesn't report speeds

Re: [PATCH 2/3] staging/rtl8192e: use s8 instead of char

[Jes Sorensen]

Arnd Bergmann <a...@arndb.de> writes:
> On Tuesday, July 19, 2016 12:05:00 PM CEST Jes Sorensen wrote:
>> Arnd Bergmann <a...@arndb.de> writes:
>> I think that would be better, albeit not a big issue. 
>
> Ok, and since Kalle applied the first patch to his tree, I'm now sending
> a series of three patches that are all for Greg, which also avoids some
> possible confusion.

Awesome!

>> I'd like to get rid of all the drivers/staging/rtl* drivers eventually 
>
> That would be great, yes.
>
> Can you clarify what the long-term plan is? I see that
> drivers/net/wireless/realtek/rtlwifi/ has most of the PCIe parts
> and one USB device (rtl8192cu/rtl8188cus) while
> drivers/net/wireless/rtl8xxx has all the USB parts including
> that one.
>
> Does that mean we want the staging drivers for PCIe devices
> to get merged into rtlwifi, and the remaining USB drivers to get
> replaced by r8xxxu?

Well it really all depends on how much time I have and how much others
step up and help contribute to the code. For rtl8xxxu my plans are as
follows:

1) rtl8188eu support, since this is the most widely distributed USB
dongle which isn't currently supported by a non staging driver. I am
currently working on this together with Andrea Merello.

2) Beacon support for IBSS and AP mode - hopefully this should make it
possible to default rtl8xxxu for rtl8192cu/rtl8188cu devices and disable
them in rtlwifi.

3) SDIO device support

4) PCI device support

5) 802.11ac device support

3/4/5 not necessarily in that order. There really is no reason why
rtl8xxxu shouldn't have SDIO and PCI device support added so the core
code can be shared.

> As one data point that I can provide (but you are probably
> aware of), I could never get my rtl8188cus stick to work with
> rtlwifi, but I found the older r8712u device to work fine with
> the staging/rtl8712 driver.

I'd love to hear if the rtl8188cus works better with rtl8xxxu. For the
rtl8712 device, rtl8192su?, then potentially that could be added to
rtl8xxxu as well, but it's not a top priority on my list right now.

Cheers,
Jes

Re: [PATCH 2/3] staging/rtl8192e: use s8 instead of char

[Jes Sorensen]

Arnd Bergmann <a...@arndb.de> writes:
> On Tuesday, July 19, 2016 11:46:04 AM CEST Jes Sorensen wrote:
>> > diff --git a/drivers/staging/rtl8192e/rtl819x_TSProc.c 
>> > b/drivers/staging/rtl8192e/rtl819x_TSProc.c
>> > index 2c8a526773ed..e0a2fe5e6148 100644
>> > --- a/drivers/staging/rtl8192e/rtl819x_TSProc.c
>> > +++ b/drivers/staging/rtl8192e/rtl819x_TSProc.c
>> > @@ -323,7 +323,7 @@ bool GetTs(struct rtllib_device *ieee, struct 
>> > ts_common_info **ppTS,
>> >   if (ieee->current_network.qos_data.supported == 0) {
>> >   UP = 0;
>> >   } else {
>> > - if (!IsACValid(TID)) {
>> > + if (!IsACValid((s8)TID)) {
>> >   netdev_warn(ieee->dev, "%s(): TID(%d) is not 
>> > valid\n",
>> >   __func__, TID);
>> >   return false;
>> 
>> TID is a 4-bit field, it should never go negative. The cast to s8 seems
>> wrong to me, if anything it should be using u8. I do realize the macro
>> IsACValid checks against negative too, but that just looks silly to me.
>
> Ok, I'll remove the extra comparison then to avoid the warning:
>
> staging/rtl8192e/rtl819x_TSProc.c:326:14: error: comparison is always
> true due to limited range of data type [-Werror=type-limits]
>
> I guess it should be a separate patch. I had just stumbled over the
> same thing before resending the patch but decided not to change it
> to keep the patch simple.

I think that would be better, albeit not a big issue. I'd like to get
rid of all the drivers/staging/rtl* drivers eventually :)

Cheers,
Jes

Re: [PATCH 3/3] staging/rtl8192u: use s8 instead of char

[Jes Sorensen]

Arnd Bergmann <a...@arndb.de> writes:
> Compiling the rtlwifi drivers for ARM with gcc -Wextra warns about lots of
> incorrect code that results from 'char' being unsigned here, e.g.
>
> staging/rtl8192u/r8192U_core.c:4150:16: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> staging/rtl8192u/r8192U_dm.c:646:50: error: comparison is always false due to 
> limited range of data type [-Werror=type-limits]
>
> This patch changes all uses of 'char' in this driver that refer to
> 8-bit integers to use 's8' instead, which is signed on all architectures.
>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> ---
>  drivers/staging/rtl8192u/ieee80211/ieee80211.h |  4 ++--
>  drivers/staging/rtl8192u/r8192U.h  |  4 ++--
>  drivers/staging/rtl8192u/r8192U_core.c | 14 +++---
>  3 files changed, 11 insertions(+), 11 deletions(-)

Looks good to me

Acked-by: Jes Sorensen <jes.soren...@redhat.com>

Re: [PATCH 2/3] staging/rtl8192e: use s8 instead of char

[Jes Sorensen]

Arnd Bergmann  writes:
> Compiling the rtlwifi drivers for ARM with gcc -Wextra warns about lots of
> incorrect code that results from 'char' being unsigned here, e.g.
>
> staging/rtl8192e/rtl8192e/r8192E_phy.c:1072:36: error: comparison is always 
> false due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtl8192e/r8192E_phy.c:1104:36: error: comparison is always 
> false due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtl8192e/rtl_core.c:1987:16: error: comparison is always 
> false due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtl8192e/rtl_dm.c:782:37: error: comparison is always false 
> due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtl819x_TSProc.c:326:14: error: comparison is always true 
> due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtllib_softmac_wx.c:465:16: error: comparison is always 
> false due to limited range of data type [-Werror=type-limits]
>
> This patch changes all uses of 'char' in this driver that refer to
> 8-bit integers to use 's8' instead, which is signed on all architectures.
>
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c | 8 
>  drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c | 2 +-
>  drivers/staging/rtl8192e/rtl8192e/rtl_core.c   | 6 +++---
>  drivers/staging/rtl8192e/rtl8192e/rtl_core.h   | 8 
>  drivers/staging/rtl8192e/rtl819x_TSProc.c  | 2 +-
>  5 files changed, 13 insertions(+), 13 deletions(-)
>

Most of this looks fine to me. One issue stands out which I don't think
is right:

> diff --git a/drivers/staging/rtl8192e/rtl819x_TSProc.c 
> b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> index 2c8a526773ed..e0a2fe5e6148 100644
> --- a/drivers/staging/rtl8192e/rtl819x_TSProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> @@ -323,7 +323,7 @@ bool GetTs(struct rtllib_device *ieee, struct 
> ts_common_info **ppTS,
>   if (ieee->current_network.qos_data.supported == 0) {
>   UP = 0;
>   } else {
> - if (!IsACValid(TID)) {
> + if (!IsACValid((s8)TID)) {
>   netdev_warn(ieee->dev, "%s(): TID(%d) is not valid\n",
>   __func__, TID);
>   return false;

TID is a 4-bit field, it should never go negative. The cast to s8 seems
wrong to me, if anything it should be using u8. I do realize the macro
IsACValid checks against negative too, but that just looks silly to me.

Cheers,
Jes

Re: [PATCH] rtlwifi: use s8 instead of char

[Jes Sorensen]

David Laight <david.lai...@aculab.com> writes:
> From: Arnd Bergmann
>> On Wednesday, June 15, 2016 5:10:51 PM CEST Jes Sorensen wrote:
>> >
>> > Arnd,
>> >
>> > rtlwifi and rtl8xxxu are two distinct drivers managed by different
>> > people. I'd be really nice if you could split this into a per driver
>> > patch.
>> >
>> > That said, the use of char in rtl8xxxu is all as a flag indicator, so I
>> > don't think the s/char/s8/ conversion is justified. I used char rather
>> > than ugly bool to reduce the size of the struct.
>> 
>> Makes sense, I'll resend without that change. If anything, the flag
>> should become u8, not s8 anyway.
>
> Does bool:8 work ?

Maybe, but bool is such an ugly datatype, so I'd rather use the other
ones.

Jes

Re: [PATCH] rtlwifi: use s8 instead of char

[Jes Sorensen]

Arnd Bergmann  writes:
> Compiling the rtlwifi drivers for ARM with gcc -Wextra warns about lots of
> incorrect code that results from 'char' being unsigned here, e.g.
>
> realtek/rtlwifi/rc.c:113:18: error: comparison is always true due to limited 
> range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8188ee/dm.c:1070:22: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8192ce/trx.c:54:16: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8192cu/mac.c:601:16: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8192de/trx.c:53:16: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8192ee/phy.c:1268:12: error: comparison is always true due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8192se/rf.c:150:20: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8723be/dm.c:877:29: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8723be/phy.c:386:16: error: comparison is always true due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8821ae/dm.c:1514:38: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8821ae/phy.c:1558:11: error: comparison is always false 
> due to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8821ae/phy.c:386:24: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/rtl8821ae/trx.c:55:12: error: comparison is always false due 
> to limited range of data type [-Werror=type-limits]
> realtek/rtlwifi/stats.c:31:16: error: comparison is always false due to 
> limited range of data type [-Werror=type-limits]
>
> This patch changes all uses of 'char' in this driver that refer to
> 8-bit integers to use 's8' instead, which is signed on all architectures.
>
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h   |  6 +-
>  .../realtek/rtlwifi/btcoexist/halbtcoutsrc.c   |  2 +-
>  .../realtek/rtlwifi/btcoexist/halbtcoutsrc.h   |  4 +-
>  drivers/net/wireless/realtek/rtlwifi/rc.c  |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8188ee/dm.c|  4 +-
>  .../net/wireless/realtek/rtlwifi/rtl8188ee/trx.c   |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8188ee/trx.h   |  4 +-
>  .../wireless/realtek/rtlwifi/rtl8192c/dm_common.h  |  2 +-
>  .../wireless/realtek/rtlwifi/rtl8192c/phy_common.c |  4 +-
>  .../wireless/realtek/rtlwifi/rtl8192c/phy_common.h |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192ce/phy.h   |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192ce/trx.c   |  6 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192ce/trx.h   |  4 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192cu/mac.c   |  6 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192cu/mac.h   |  4 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192de/phy.c   |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192de/phy.h   |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192de/trx.c   |  6 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192de/trx.h   |  4 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192ee/phy.c   | 10 ++--
>  .../net/wireless/realtek/rtlwifi/rtl8192ee/phy.h   |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192ee/trx.c   |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192ee/trx.h   |  4 +-
>  .../net/wireless/realtek/rtlwifi/rtl8192se/rf.c|  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8723ae/trx.h   |  4 +-
>  .../net/wireless/realtek/rtlwifi/rtl8723be/dm.c|  4 +-
>  .../net/wireless/realtek/rtlwifi/rtl8723be/phy.c   |  4 +-
>  .../net/wireless/realtek/rtlwifi/rtl8723be/trx.c   |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8723be/trx.h   |  8 +--
>  .../net/wireless/realtek/rtlwifi/rtl8821ae/dm.c|  4 +-
>  .../net/wireless/realtek/rtlwifi/rtl8821ae/phy.c   | 48 +++
>  .../net/wireless/realtek/rtlwifi/rtl8821ae/phy.h   |  2 +-
>  .../net/wireless/realtek/rtlwifi/rtl8821ae/trx.c   | 12 ++--
>  .../net/wireless/realtek/rtlwifi/rtl8821ae/trx.h   | 10 ++--
>  drivers/net/wireless/realtek/rtlwifi/stats.c   |  6 +-
>  drivers/net/wireless/realtek/rtlwifi/stats.h   |  4 +-
>  drivers/net/wireless/realtek/rtlwifi/wifi.h| 68
> +++--

Arnd,

rtlwifi and rtl8xxxu are two distinct drivers managed by different
people. I'd be really nice if you could split this into a per driver
patch.

That said, the use of char in rtl8xxxu is all as a flag indicator, so I
don't think the s/char/s8/ conversion is justified. I used char rather
than ugly bool to reduce the size of the struct.

Cheers,
Jes

Re: [PATCH] rtl8xxxu: fix typo on variable name, compare against correct variable

[Jes Sorensen]

Jes Sorensen <jes.soren...@redhat.com> writes:
> Colin King <colin.k...@canonical.com> writes:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> path_b_ok is being assigned but immediately after path_a_ok is being
>> compared to the value 0x03.  This appears to be a typo on the
>> variable name, compare path_b_ok instead.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c 
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>> index fe19ace..b04cf30 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>> @@ -1149,7 +1149,7 @@ static void rtl8192eu_phy_iqcalibrate(struct 
>> rtl8xxxu_priv *priv,
>>  
>>  for (i = 0; i < retry; i++) {
>>  path_b_ok = rtl8192eu_rx_iqk_path_b(priv);
>> -if (path_a_ok == 0x03) {
>> +if (path_b_ok == 0x03) {
>>  val32 = rtl8xxxu_read32(priv,
>>  
>> REG_RX_POWER_BEFORE_IQK_B_2);
>>  result[t][6] = (val32 >> 16) & 0x3ff;
>
> Nice catch, that does indeed look like a bug!
>
> Thanks, I want to test it, but I plan to apply it to rtl8xxxu-devel
> shortly.

Tested and applied!

Thanks,
Jes

Re: [PATCH] rtl8xxxu: fix typo on variable name, compare against correct variable

[Jes Sorensen]

Colin King  writes:
> From: Colin Ian King 
>
> path_b_ok is being assigned but immediately after path_a_ok is being
> compared to the value 0x03.  This appears to be a typo on the
> variable name, compare path_b_ok instead.
>
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> index fe19ace..b04cf30 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> @@ -1149,7 +1149,7 @@ static void rtl8192eu_phy_iqcalibrate(struct 
> rtl8xxxu_priv *priv,
>  
>   for (i = 0; i < retry; i++) {
>   path_b_ok = rtl8192eu_rx_iqk_path_b(priv);
> - if (path_a_ok == 0x03) {
> + if (path_b_ok == 0x03) {
>   val32 = rtl8xxxu_read32(priv,
>   
> REG_RX_POWER_BEFORE_IQK_B_2);
>   result[t][6] = (val32 >> 16) & 0x3ff;

Nice catch, that does indeed look like a bug!

Thanks, I want to test it, but I plan to apply it to rtl8xxxu-devel
shortly.

Cheers,
Jes

Re: [PATCH] rtl8xxxu: hide unused tables

[Jes Sorensen]

Kalle Valo <kv...@codeaurora.org> writes:
> Jes Sorensen <jes.soren...@redhat.com> writes:
>
>> Arnd Bergmann <a...@arndb.de> writes:
>>> The references to some arrays in the rtl8xxxu driver were moved inside
>>> of an #ifdef, but the symbols remain outside, resulting in build warnings:
>>>
>>> rtl8xxxu/rtl8xxxu.c:1506:33: error:
>>> 'rtl8188ru_radioa_1t_highpa_table' defined but not used
>>> rtl8xxxu/rtl8xxxu.c:1431:33: error:
>>> 'rtl8192cu_radioa_1t_init_table' defined but not used
>>> rtl8xxxu/rtl8xxxu.c:1407:33: error:
>>> 'rtl8192cu_radiob_2t_init_table' defined but not used
>>> rtl8xxxu/rtl8xxxu.c:1332:33: error:
>>> 'rtl8192cu_radioa_2t_init_table' defined but not used
>>> rtl8xxxu/rtl8xxxu.c:239:35: error: 'rtl8192c_power_base' defined but not 
>>> used
>>> rtl8xxxu/rtl8xxxu.c:217:35: error: 'rtl8188r_power_base' defined but not 
>>> used
>>>
>>> This adds an extra #ifdef around them to shut up the warnings.
>>>
>>> Signed-off-by: Arnd Bergmann <a...@arndb.de>
>>> Fixes: 2fc0b8e5a17d ("rtl8xxxu: Add TX power base values for gen1 parts")
>>> Fixes: 4062b8ffec36 ("rtl8xxxu: Move PHY RF init into device
>>> specific functions")
>>> ---
>>>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 4 
>>>  1 file changed, 4 insertions(+)
>>
>> I'll apply it to my tree!
>
> Actually I would prefer to apply this directly to wireless-drivers-next
> so that the warnings are quickly fixed.

Thats fine with me, I already applied it here before I started doing any
of the refactoring work, so we should be in sync.

Cheers,
Jes

Re: [PATCH] rtl8xxxu: hide unused tables

[Jes Sorensen]

Arnd Bergmann  writes:
> The references to some arrays in the rtl8xxxu driver were moved inside
> of an #ifdef, but the symbols remain outside, resulting in build warnings:
>
> rtl8xxxu/rtl8xxxu.c:1506:33: error: 'rtl8188ru_radioa_1t_highpa_table' 
> defined but not used
> rtl8xxxu/rtl8xxxu.c:1431:33: error: 'rtl8192cu_radioa_1t_init_table' defined 
> but not used
> rtl8xxxu/rtl8xxxu.c:1407:33: error: 'rtl8192cu_radiob_2t_init_table' defined 
> but not used
> rtl8xxxu/rtl8xxxu.c:1332:33: error: 'rtl8192cu_radioa_2t_init_table' defined 
> but not used
> rtl8xxxu/rtl8xxxu.c:239:35: error: 'rtl8192c_power_base' defined but not used
> rtl8xxxu/rtl8xxxu.c:217:35: error: 'rtl8188r_power_base' defined but not used
>
> This adds an extra #ifdef around them to shut up the warnings.
>
> Signed-off-by: Arnd Bergmann 
> Fixes: 2fc0b8e5a17d ("rtl8xxxu: Add TX power base values for gen1 parts")
> Fixes: 4062b8ffec36 ("rtl8xxxu: Move PHY RF init into device specific 
> functions")
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 4 
>  1 file changed, 4 insertions(+)

I'll apply it to my tree!

Thanks,
Jes

Re: [PATCH] rtl8xxxu: fix uninitialized return value in ret

[Jes Sorensen]

Colin King  writes:
> From: Colin Ian King 
>
> several functions are not initializing a return status in ret
> resulting in garbage to be returned instead of 0 for success.
> Currently, the calls to these functions are not checking the
> return, however, it seems prudent to return the correct status
> in case they are to be checked at a later date.
>
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Thanks for the patch! I'm surprised the compiler didn't warn about this.

I'll add it to my queue for rtl8xxxu.

Cheers,
Jes

>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> index abdff45..9262aad 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> @@ -5231,7 +5231,7 @@ static void rtl8xxxu_set_ampdu_min_space(struct 
> rtl8xxxu_priv *priv, u8 density)
>  static int rtl8xxxu_active_to_emu(struct rtl8xxxu_priv *priv)
>  {
>   u8 val8;
> - int count, ret;
> + int count, ret = 0;
>  
>   /* Start of rtl8723AU_card_enable_flow */
>   /* Act to Cardemu sequence*/
> @@ -5281,7 +5281,7 @@ static int rtl8723bu_active_to_emu(struct rtl8xxxu_priv 
> *priv)
>   u8 val8;
>   u16 val16;
>   u32 val32;
> - int count, ret;
> + int count, ret = 0;
>  
>   /* Turn off RF */
>   rtl8xxxu_write8(priv, REG_RF_CTRL, 0);
> @@ -5338,7 +5338,7 @@ static int rtl8xxxu_active_to_lps(struct rtl8xxxu_priv 
> *priv)
>  {
>   u8 val8;
>   u8 val32;
> - int count, ret;
> + int count, ret = 0;
>  
>   rtl8xxxu_write8(priv, REG_TXPAUSE, 0xff);

Re: pull-request: wireless-drivers-next 2016-03-14

[Jes Sorensen]

Kalle Valo  writes:
> David Miller  writes:
>
>> From: Kalle Valo 
>> Date: Mon, 14 Mar 2016 10:31:48 +0200
>>
>>> I know I'm late now that merge window was opened yesterday but here's
>>> one more set of patches I would like to get to 4.6 still. There isn't
>>> anything controversial so I hope this should be still safe to pull. The
>>> patches have been in linux-next since Friday and I haven't seen any
>>> reports about issues. But if you think it's too late just let me know
>>> and I'll resubmit these for 4.7.
>>> 
>>> The most notable part here of course is rtl8xxxu with over 100 patches.
>>> As the driver is new and under heavy development I think they are ok to
>>> take still. Otherwise there are mostly fixes with an exception of adding
>>> a new debugfs file to wl18xx.
>>> 
>>> Please let me know if you have any problems.
>>
>> Pulled, thanks.
>
> Great, thanks a lot.
>
>> I really like Jes's work and I wish you had integrated it several
>> months ago, instead of sloshing him needlessly through a non-stop
>> cycle of very nit-picky issues, just FYI.
>
> I also like his work and I'm sorry for being too nit-picky. I have tried
> to be extra careful with the patches I send to you, especially with new
> drivers, and I guess I have been too pedantic. I'll try to lower the bar
> to a more reasonable level.
>
> But I actually started to wonder what you actually mean and checked the
> dates of initial rtl8xxxu submission from patchwork:
>
> 2015-08-29 v1
> 2015-08-30 v2
> 2015-10-15 v3
> 2015-10-21 applied 26f1fad29ad9 to w-d-next for v4.4
>
> Two months is quite long for a good driver like this but IIRC the
> initial commit was pending wireless-drivers directory reorganisation,
> and that just took too long on my side.

With all the trying to break down the patches and reorganizing them into
smaller blocks (disentangling 8723bu and 8192eu) to be submitted
independently, I have now ended up with a tree that simply is
unmergeable. I have spent about three days trying to roll back merges
and pop off my devel changes to allow it to merge, and I give up.

As a result I have created a new branch off wireless-drivers-next and
cherry picked all my devel patches on top of this. The new branch is
rtl8xxxu-devel. The old branch rtl8xxxu-nextgen I am going to let go
stale, but I will keep it in place as it contains all the old devel
history for the original driver prior to the initial upstream submission
of rtl8xxxu.

For anyone following rtl8xxxu development and/or submitting patches,
please submit against the new branch from now on.

Thanks,
Jes

Re: [PATCH] acenic: SET_NETDEV_DEV is always there these days

[Jes Sorensen]

 Geert == Geert Uytterhoeven [EMAIL PROTECTED] writes:

Geert acenic: SET_NETDEV_DEV is always there these days

Geert Signed-off-by: Geert Uytterhoeven [EMAIL PROTECTED]

Signed-off-by: Jes Sorensen [EMAIL PROTECTED]

Geert --- Disclaimer: not tested at all

Geert --- a/drivers/net/acenic.c +++ b/drivers/net/acenic.c @@ -159,10
Geert +159,6 @@ static struct pci_device_id acenic_pci_t };
Geert MODULE_DEVICE_TABLE(pci, acenic_pci_tbl);
 
Geert -#ifndef SET_NETDEV_DEV -#define SET_NETDEV_DEV(net, pdev) do{}
Geert while(0) -#endif - #define ace_sync_irq(irq) synchronize_irq(irq)
 
Geert  #ifndef offset_in_page

Geert Gr{oetje,eeting}s,

Geert  Geert

Geert -- Geert Uytterhoeven -- Sony Network and Software Technology
Geert Center Europe (NSCE) [EMAIL PROTECTED] --- The
Geert Corporate Village, Da Vincilaan 7-D1 Voice +32-2-7008453 Fax
Geert +32-2-7008622  B-1935 Zaventem, Belgium
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 04/11] drivers/net/acenic.c Removal of old code

[Jes Sorensen]

 akpm == akpm  [EMAIL PROTECTED] writes:

akpm From: Michal Piotrowski [EMAIL PROTECTED]
akpm Signed-off-by: Michal Piotrowski [EMAIL PROTECTED]
akpm Cc: Jeff Garzik [EMAIL PROTECTED]
akpm Cc: Jes Sorensen [EMAIL PROTECTED]
akpm Signed-off-by: Andrew Morton [EMAIL PROTECTED]

Signed-off-by: Jes Sorensen [EMAIL PROTECTED]

akpm  drivers/net/acenic.c | 4  1 file changed, 4 deletions(-)

akpm diff -puN
akpm drivers/net/acenic.c~drivers-net-acenicc-removal-of-old-code
akpm drivers/net/acenic.c ---
akpm a/drivers/net/acenic.c~drivers-net-acenicc-removal-of-old-code +++
akpm a/drivers/net/acenic.c @@ -163,11 +163,7 @@
akpm MODULE_DEVICE_TABLE(pci, acenic_pci_tbl) #define
akpm SET_NETDEV_DEV(net, pdev) do{} while(0) #endif
 
akpm -#if LINUX_VERSION_CODE = 0x2051c #define ace_sync_irq(irq)
akpm synchronize_irq(irq) -#else -#define ace_sync_irq(irq)
akpm synchronize_irq() -#endif
 
akpm  #ifndef offset_in_page #define offset_in_page(ptr) ((unsigned
akpm long)(ptr)  ~PAGE_MASK) _
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Private driver support emails should be avoided.

[Jes Sorensen]

 Jeff == Jeff Garzik [EMAIL PROTECTED] writes:

Jeff Just a reminder to folks out there...  Don't send private driver
Jeff support emails.

Jeff I receive a lot of email.  A lot.  Not as much as Linus or
Jeff Andrew probably, but still sizable.  And quite regularly, I will
Jeff receive a private email asking for help on a SATA or net driver
Jeff problem.  The poster will include various details on their
Jeff problem, and ask for help debugging the problem.  While the
Jeff email will indeed be skimmed -- looking at a mass of bug
Jeff reports, one can better detect patterns -- the people seeking
Jeff support privately will almost NEVER RECEIVE A RESPONSE.

Jeff,

Very sane - I get some of it too, though less so these days :)

What about writing this up and sticking it under
Documentation/Reporting-Bugs or something so there's an easy reference
to it?

Cheers,
Jes
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] acenic: fix section mismatches

[Jes Sorensen]

 Randy == Randy Dunlap [EMAIL PROTECTED] writes:

Randy From: Randy Dunlap [EMAIL PROTECTED] Fix section mismatches
Randy in acenic driver: WARNING: drivers/net/acenic.o - Section
Randy mismatch: reference to .init.data:tigon2FwText from .text between
Randy 'acenic_probe_one' (at offset 0x2409) and 'ace_interrupt'
Randy WARNING: drivers/net/acenic.o - Section mismatch: reference to
Randy .init.data:tigon2FwRodata from .text between 'acenic_probe_one'
Randy (at offset 0x2422) and 'ace_interrupt'

Randy Signed-off-by: Randy Dunlap [EMAIL PROTECTED] ---

Acked-by: Jes Sorensen [EMAIL PROTECTED]

Jes
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kmalloc_node returns unaligned memory

[Jes Sorensen]

 Olaf == Olaf Hering [EMAIL PROTECTED] writes:

Olaf kmalloc_node returns unaligned pointers on powerpc, when
Olaf CONFIG_DEBUG_SLAB is enabled. This makes iptables very
Olaf unhappy. It checks the alignment in
Olaf net/ipv6/netfilter/ip6_tables.c:check_entry_size_and_hooks().
Olaf __alignof__(struct ip6t_entry) returns 8. But returned pointers
Olaf from xt_alloc_table_info() are unaligned:

Hi Olaf,

I believe this is expected behavior ;-(

We have the same problem with the XPC driver for the SN2 which resulted
in a wrapper macro being created for it.

Some sort of SLAB_HWCACHE_ALIGN flag to Slab that was always respected
for this would be nice.

Cheers,
Jes
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html