bnx2x: fixes

drivers/net/ethernet/broadcom/bnx2x/bnx2x.h struct bnx2x { · line 1834

Quoted 2 lines · On Tue, 13 Aug 2013 02:24:59 +0300 "Dmitry Kravkov" <[email protected]> wrote:

drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2 -0

+	bool					stats_started;
+	struct semaphore			stats_sema;

Is there a reason to use a counting semaphore? Do you expect it to be held across user process boundary? or want count > 1?

Use of semaphores as a locking primitive is discouraged, instead us a mutex.

From: Stephen Hemminger <[email protected]>
Date: Mon, 12 Aug 2013 17:12:08 -0700

Quoted 9 lines

drivers/net/ethernet/broadcom/bnx2x/bnx2x.h struct bnx2x { · line 1834

Quoted 2 lines · On Tue, 13 Aug 2013 02:24:59 +0300 "Dmitry Kravkov" <[email protected]> wrote:

drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2 -0

+	bool					stats_started;
+	struct semaphore			stats_sema;

Is there a reason to use a counting semaphore? Do you expect it to be held across user process boundary? or want count > 1?

Quoted 2 lines

Use of semaphores as a locking primitive is discouraged, instead us a mutex.

Would you please look at the feedback I gave these guys to the previous iteration of these changes?

They were using custom locking primitives and semaphores gave the best approximation to the semantics they were looking for.

Hello Dave

Please consider applying the series of bnx2x fixes to net:
* statistics may cause FW assert
* missing fairness configuration in DCB flow
* memory leak in sriov related part
* Illegal PTE access
* Pagefault crash in shutdown flow with cnic
v1->v2
* fixed sparse error pointed by Joe Perches
* added missing signed-off from Sergei Shtylyov
v2->v3

* added missing signed-off from Sergei Shtylyov
* fixed formatting from Sergei Shtylyov
v3->v4
* patch 1/6: fixed declaration order
* patch 2/6 replaced with: protect flows using set_bit constraints
v4->v5
* patch 2/6: replace proprietary locking with semaphore
* droped 1/6: since adds redundant code from Benjamin Poirier

I'm confused. Wasn't "[PATCH net v4 1/6] bnx2x: properly initialize statistic counters" supposed to fix a race condition? According to earlier communication with Ariel:
In this issue a race condition at driver startup causes a second statistics query to be sent before the first one completes, resulting in a firmware assert and a stuck chip. A patch was sent upstream fixing this:
http://patchwork.ozlabs.org/patch/264810/

Quoted 8 lines

I'm confused. Wasn't "[PATCH net v4 1/6] bnx2x: properly initialize statistic counters" supposed to fix a race condition? According to earlier communication with Ariel:
In this issue a race condition at driver startup causes a second statistics query to be sent before the first one completes, resulting in a firmware assert and a stuck chip. A patch was sent upstream fixing this:
http://patchwork.ozlabs.org/patch/264810/

As I explicitly mentioned in the communique which you quoted above, the patch was sent but not yet accepted. This is precisely the upstream process - patches are being sent, reviewed and sometimes rejected and revised. If you have further questions please address them to me - the technical forum is no place for this kind of discussion. Thanks,
Ariel

-----Original Message-----
From: [email protected] [mailto:netdev- [email protected]] On Behalf Of Benjamin Poirier
Sent: Tuesday, August 13, 2013 5:07 PM
To: Ariel Elior
Cc: Dmitry Kravkov; [email protected]; [email protected]; Eilon Greenstein
Subject: Re: [PATCH net v5 0/5] bnx2x: fixes

Quoted 10 lines · Ariel Elior

Quoted 8 lines

I'm confused. Wasn't "[PATCH net v4 1/6] bnx2x: properly initialize statistic counters" supposed to fix a race condition? According to earlier communication with Ariel:
In this issue a race condition at driver startup causes a second statistics query to be sent before the first one completes, resulting in a firmware assert and a stuck chip. A patch was sent upstream fixing this:
http://patchwork.ozlabs.org/patch/264810/

As I explicitly mentioned in the communique which you quoted above,

the patch was sent but not yet accepted.

Quoted 1 line

This is precisely the upstream process - patches are being sent, reviewed

and sometimes rejected and revised.

Quoted 1 line

If you have further questions please address them to me - the technical

forum is no place for this kind of discussion.

Quoted 3 lines

Thanks,
Ariel

It's a technical question about a patch which was sent upstream. Where should the discussion happen if not upstream?

Let me rephrase my question:

I'm confused. I thought that "[PATCH net v4 1/6] bnx2x: properly initialize statistic counters" was meant to fix a race condition at driver startup which causes a second statistics query to be sent before the first one completes, resulting in a firmware assert and a stuck chip. Am I mistaken and there is no such race condition, or is it addressed by the other patches in this series?

Quoted 9 lines

Let me rephrase my question:

I'm confused. I thought that "[PATCH net v4 1/6] bnx2x: properly initialize statistic counters" was meant to fix a race condition at driver startup which causes a second statistics query to be sent before the first one completes, resulting in a firmware assert and a stuck chip. Am I mistaken and there is no such race condition, or is it addressed by the other patches in this series?

There is such condition, but it's not caused by wrongly initialized counted - the initialization done in a correct way. Patch "[PATCH net v5 1/5] bnx2x: protect different statistics flows" - prevents from two outstanding queries to be sent simultaneously.

Please consider applying the series of bnx2x fixes to net:
* statistics may cause FW assert
* missing fairness configuration in DCB flow
* memory leak in sriov related part
* Illegal PTE access
* Pagefault crash in shutdown flow with cnic