Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang

[Jes Sorensen]

On 4/17/21 3:24 PM, Gustavo A. R. Silva wrote:
> 
> 
> On 4/17/21 13:29, Jes Sorensen wrote:
>> On 3/10/21 3:59 PM, Kees Cook wrote:
>>> On Wed, Mar 10, 2021 at 02:51:24PM -0500, Jes Sorensen wrote:
>>>> On 3/10/21 2:45 PM, Kees Cook wrote:
>>>>> On Wed, Mar 10, 2021 at 02:31:57PM -0500, Jes Sorensen wrote:
>>>>>> On 3/10/21 2:14 PM, Kees Cook wrote:
>>>>>>> Hm, this conversation looks like a miscommunication, mainly? I see
>>>>>>> Gustavo, as requested by many others[1], replacing the fallthrough
>>>>>>> comments with the "fallthrough" statement. (This is more than just a
>>>>>>> "Clang doesn't parse comments" issue.)
>>>>>>>
>>>>>>> This could be a tree-wide patch and not bother you, but Greg KH has
>>>>>>> generally advised us to send these changes broken out. Anyway, this
>>>>>>> change still needs to land, so what would be the preferred path? I think
>>>>>>> Gustavo could just carry it for Linus to merge without bothering you if
>>>>>>> that'd be preferred?
>>>>>>
>>>>>> I'll respond with the same I did last time, fallthrough is not C and
>>>>>> it's ugly.
>>>>>
>>>>> I understand your point of view, but this is not the consensus[1] of
>>>>> the community. "fallthrough" is a macro, using the GCC fallthrough
>>>>> attribute, with the expectation that we can move to the C17/C18
>>>>> "[[fallthrough]]" statement once it is finalized by the C standards
>>>>> body.
>>>>
>>>> I don't know who decided on that, but I still disagree. It's an ugly and
>>>> pointless change that serves little purpose. We shouldn't have allowed
>>>> the ugly /* fall-through */ comments in either, but at least they didn't
>>>> mess with the code. I guess when you give someone an inch, they take a 
>>>> mile.
>>>>
>>>> Last time this came up, the discussion was that clang refused to fix
>>>> their brokenness and therefore this nonsense was being pushed into the
>>>> kernel. It's still a pointless argument, if clang can't fix it's crap,
>>>> then stop using it.
>>>>
>>>> As Kalle correctly pointed out, none of the previous comments to this
>>>> were addressed, the patches were just reposted as fact. Not exactly a
>>>> nice way to go about it either.
>>>
>>> Do you mean changing the commit log to re-justify these changes? I
>>> guess that could be done, but based on the thread, it didn't seem to
>>> be needed. The change is happening to match the coding style consensus
>>> reached to give the kernel the flexibility to move from a gcc extension
>>> to the final C standards committee results without having to do treewide
>>> commits again (i.e. via the macro).
>>
>> No, I am questioning why Gustavo continues to push this nonsense that
>> serves no purpose whatsoever. In addition he has consistently ignored
>> comments and just keep reposting it. But I guess that is how it works,
>> ignore feedback, repost junk, repeat.
> 
> I was asking for feedback here[1] and here[2] after people (you and Kalle)
> commented on this patch. How is that ignoring people? And -again- why
> people ignored my requests for feedback in this conversation? It's a mystery
> to me, honestly.

All you did was post a pointer to the fact that some other people
couldn't be bothered speaking out against the patch, and let it go in.
You haven't addressed any of the original concerns raised.

The big mistake here was of course to allow the pointless /* fallthrough
*/ changes to go in in the first place.

Jes

Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang

[Jes Sorensen]

On 4/17/21 8:09 PM, Joe Perches wrote:
> On Sat, 2021-04-17 at 14:30 -0400, Jes Sorensen wrote:
>> On 4/17/21 1:52 PM, Kalle Valo wrote:
>>> "Gustavo A. R. Silva"  wrote:
>>>
>>>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>>>> multiple warnings by replacing /* fall through */ comments with
>>>> the new pseudo-keyword macro fallthrough; instead of letting the
>>>> code fall through to the next case.
>>>>
>>>> Notice that Clang doesn't recognize /* fall through */ comments as
>>>> implicit fall-through markings.
>>>>
>>>> Link: https://github.com/KSPP/linux/issues/115
>>>> Signed-off-by: Gustavo A. R. Silva 
>>>
>>> Patch applied to wireless-drivers-next.git, thanks.
>>>
>>> bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang
>>>
>>
>> Sorry this junk patch should not have been applied.
> 
> I don't believe it's a junk patch.
> I believe your characterization of it as such is flawed.
> 
> You don't like the style, that's fine, but:
> 
> Any code in the kernel should not be a unique style of your own choosing
> when it could cause various compilers to emit unnecessary warnings.
> 
> Please remember the kernel code base is a formed by a community with a
> nominally generally accepted style.  There is a real desire in that
> community to both enable compiler warnings that might show defects and
> simultaneously avoid unnecessary compiler warnings.
> 
> This particular change just avoids a possible compiler warning.

Please go back and look at the thread. This patch fixes nothing, it
mutilates the code by introducing non C for the sole purpose of avoiding
to work with the Clang community to fix their broken compiler. The
author of this patch ignored previous feedback and just reposted the
same patch unaltered and when it was called out, the answer was other
people was fine with it. None of the issues raised have been addressed,
so yes, that makes the patch junk.

Jes

Re: [PATCH] rtl8xxxu: Fix device info for RTL8192EU devices

[Jes Sorensen]

On 3/23/21 3:36 PM, Pascal Terjan wrote:
> Based on 2001:3319 and 2357:0109 which I used to test the fix and
> 0bda:818b and 2357:0108 for which I found efuse dumps online.
> 
> == 2357:0109 ==
> === Before ===
> Vendor: Realtek
> Product: \x03802.11n NI
> Serial:
> === After ===
> Vendor: Realtek
> Product: 802.11n NIC
> Serial not available.
> 
> == 2001:3319 ==
> === Before ===
> Vendor: Realtek
> Product: Wireless N
> Serial: no USB Adap
> === After ===
> Vendor: Realtek
> Product: Wireless N Nano USB Adapter
> Serial not available.
> 
> Signed-off-by: Pascal Terjan 
> ---
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  | 11 ++--
>  .../realtek/rtl8xxxu/rtl8xxxu_8192e.c | 53 ---
>  2 files changed, 50 insertions(+), 14 deletions(-)

This makes sense, you may want to account for the total length of the
record though, see below.

Some cosmetic nits too.

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index d6d1be4169e5..acb6b0cd3667 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -853,15 +853,10 @@ struct rtl8192eu_efuse {
>   u8 usb_optional_function;
>   u8 res9[2];
>   u8 mac_addr[ETH_ALEN];  /* 0xd7 */
> - u8 res10[2];
> - u8 vendor_name[7];
> - u8 res11[2];
> - u8 device_name[0x0b];   /* 0xe8 */
> - u8 res12[2];
> - u8 serial[0x0b];/* 0xf5 */
> - u8 res13[0x30];
> + u8 device_info[80];
> + u8 res11[3];
>   u8 unknown[0x0d];   /* 0x130 */
> - u8 res14[0xc3];
> + u8 res12[0xc3];
>  };
>  
>  struct rtl8xxxu_reg8val {
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> index cfe2dfdae928..9c5fad49ed2a 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> @@ -554,9 +554,39 @@ rtl8192e_set_tx_power(struct rtl8xxxu_priv *priv, int 
> channel, bool ht40)
>   }
>  }
>  
> +static void rtl8192eu_log_device_info(struct rtl8xxxu_priv *priv,
> +   char *record_name,
> +   char **record)
> +{
> + /* A record is [ total length | 0x03 | value ] */
> + unsigned char l = (*record)[0];

These parenthesis make no sense.

> +
> + /* The whole section seems to be 80 characters so a record should not
> +  * be able to be that large.
> +  */

Please respect the comment formatting of the driver, ie
/*
 * Foo
 */


> + if (l > 80) {
> + dev_warn(>udev->dev,
> +  "invalid record length %d while parsing \"%s\".\n",
> +  l, record_name);
> + return;
> + }

The 80 check is only valid for the first entry, consecutive entries are
already advanced. Maybe switch it over to use an index to address into
the record keep an index and just pass in efuse->device_info instead.

> +
> + if (l >= 2) {
> + char value[80];
> +
> + memcpy(value, &(*record)[2], l - 2);
> + value[l - 2] = '\0';
> + dev_info(>udev->dev, "%s: %s\n", record_name, value);
> + *record = *record + l;
> + } else {
> + dev_info(>udev->dev, "%s not available.\n", record_name);
> + }
> +}
> +
>  static int rtl8192eu_parse_efuse(struct rtl8xxxu_priv *priv)
>  {
>   struct rtl8192eu_efuse *efuse = >efuse_wifi.efuse8192eu;
> + char *record = efuse->device_info;
>   int i;
>  
>   if (efuse->rtl_id != cpu_to_le16(0x8129))
> @@ -604,12 +634,23 @@ static int rtl8192eu_parse_efuse(struct rtl8xxxu_priv 
> *priv)
>   priv->has_xtalk = 1;
>   priv->xtalk = priv->efuse_wifi.efuse8192eu.xtal_k & 0x3f;
>  
> - dev_info(>udev->dev, "Vendor: %.7s\n", efuse->vendor_name);
> - dev_info(>udev->dev, "Product: %.11s\n", efuse->device_name);
> - if (memchr_inv(efuse->serial, 0xff, 11))
> - dev_info(>udev->dev, "Serial: %.11s\n", efuse->serial);
> - else
> - dev_info(>udev->dev, "Serial not available.\n");
> + /* device_info section seems to be laid out as records
> +  * [ total length | 0x03 | value ] so:
> +  * - vendor length + 2
> +  * - 0x03
> +  * - vendor string (not null terminated)
> +  * - product length + 2
> +  * - 0x03
> +  * - product string (not null terminated)
> +  * Then there is one or 2 0x00 on all the 4 devices I own or found
> +  * dumped online.
> +  * As previous version of the code handled an optional serial
> +  * string, I now assume there may be a third record if the
> +  * length is not 0.
> +  */
> + rtl8192eu_log_device_info(priv, "Vendor", );
> + rtl8192eu_log_device_info(priv, "Product", );
> + rtl8192eu_log_device_info(priv, "Serial", );
>  
>   if 

Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang

[Jes Sorensen]

On 4/17/21 1:52 PM, Kalle Valo wrote:
> "Gustavo A. R. Silva"  wrote:
> 
>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>> multiple warnings by replacing /* fall through */ comments with
>> the new pseudo-keyword macro fallthrough; instead of letting the
>> code fall through to the next case.
>>
>> Notice that Clang doesn't recognize /* fall through */ comments as
>> implicit fall-through markings.
>>
>> Link: https://github.com/KSPP/linux/issues/115
>> Signed-off-by: Gustavo A. R. Silva 
> 
> Patch applied to wireless-drivers-next.git, thanks.
> 
> bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang
> 

Sorry this junk patch should not have been applied.

Jes

Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang

[Jes Sorensen]

On 3/10/21 3:59 PM, Kees Cook wrote:
> On Wed, Mar 10, 2021 at 02:51:24PM -0500, Jes Sorensen wrote:
>> On 3/10/21 2:45 PM, Kees Cook wrote:
>>> On Wed, Mar 10, 2021 at 02:31:57PM -0500, Jes Sorensen wrote:
>>>> On 3/10/21 2:14 PM, Kees Cook wrote:
>>>>> Hm, this conversation looks like a miscommunication, mainly? I see
>>>>> Gustavo, as requested by many others[1], replacing the fallthrough
>>>>> comments with the "fallthrough" statement. (This is more than just a
>>>>> "Clang doesn't parse comments" issue.)
>>>>>
>>>>> This could be a tree-wide patch and not bother you, but Greg KH has
>>>>> generally advised us to send these changes broken out. Anyway, this
>>>>> change still needs to land, so what would be the preferred path? I think
>>>>> Gustavo could just carry it for Linus to merge without bothering you if
>>>>> that'd be preferred?
>>>>
>>>> I'll respond with the same I did last time, fallthrough is not C and
>>>> it's ugly.
>>>
>>> I understand your point of view, but this is not the consensus[1] of
>>> the community. "fallthrough" is a macro, using the GCC fallthrough
>>> attribute, with the expectation that we can move to the C17/C18
>>> "[[fallthrough]]" statement once it is finalized by the C standards
>>> body.
>>
>> I don't know who decided on that, but I still disagree. It's an ugly and
>> pointless change that serves little purpose. We shouldn't have allowed
>> the ugly /* fall-through */ comments in either, but at least they didn't
>> mess with the code. I guess when you give someone an inch, they take a mile.
>>
>> Last time this came up, the discussion was that clang refused to fix
>> their brokenness and therefore this nonsense was being pushed into the
>> kernel. It's still a pointless argument, if clang can't fix it's crap,
>> then stop using it.
>>
>> As Kalle correctly pointed out, none of the previous comments to this
>> were addressed, the patches were just reposted as fact. Not exactly a
>> nice way to go about it either.
> 
> Do you mean changing the commit log to re-justify these changes? I
> guess that could be done, but based on the thread, it didn't seem to
> be needed. The change is happening to match the coding style consensus
> reached to give the kernel the flexibility to move from a gcc extension
> to the final C standards committee results without having to do treewide
> commits again (i.e. via the macro).

No, I am questioning why Gustavo continues to push this nonsense that
serves no purpose whatsoever. In addition he has consistently ignored
comments and just keep reposting it. But I guess that is how it works,
ignore feedback, repost junk, repeat.

Jes

Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang

[Jes Sorensen]

On 3/10/21 2:45 PM, Kees Cook wrote:
> On Wed, Mar 10, 2021 at 02:31:57PM -0500, Jes Sorensen wrote:
>> On 3/10/21 2:14 PM, Kees Cook wrote:
>>> Hm, this conversation looks like a miscommunication, mainly? I see
>>> Gustavo, as requested by many others[1], replacing the fallthrough
>>> comments with the "fallthrough" statement. (This is more than just a
>>> "Clang doesn't parse comments" issue.)
>>>
>>> This could be a tree-wide patch and not bother you, but Greg KH has
>>> generally advised us to send these changes broken out. Anyway, this
>>> change still needs to land, so what would be the preferred path? I think
>>> Gustavo could just carry it for Linus to merge without bothering you if
>>> that'd be preferred?
>>
>> I'll respond with the same I did last time, fallthrough is not C and
>> it's ugly.
> 
> I understand your point of view, but this is not the consensus[1] of
> the community. "fallthrough" is a macro, using the GCC fallthrough
> attribute, with the expectation that we can move to the C17/C18
> "[[fallthrough]]" statement once it is finalized by the C standards
> body.

I don't know who decided on that, but I still disagree. It's an ugly and
pointless change that serves little purpose. We shouldn't have allowed
the ugly /* fall-through */ comments in either, but at least they didn't
mess with the code. I guess when you give someone an inch, they take a mile.

Last time this came up, the discussion was that clang refused to fix
their brokenness and therefore this nonsense was being pushed into the
kernel. It's still a pointless argument, if clang can't fix it's crap,
then stop using it.

As Kalle correctly pointed out, none of the previous comments to this
were addressed, the patches were just reposted as fact. Not exactly a
nice way to go about it either.

Jes

Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang

[Jes Sorensen]

On 3/10/21 2:14 PM, Kees Cook wrote:
> On Fri, Mar 05, 2021 at 03:40:33PM +0200, Kalle Valo wrote:
>> "Gustavo A. R. Silva"  writes:
>>
>>> In preparation to enable -Wimplicit-fallthrough for Clang, fix
>>> multiple warnings by replacing /* fall through */ comments with
>>> the new pseudo-keyword macro fallthrough; instead of letting the
>>> code fall through to the next case.
>>>
>>> Notice that Clang doesn't recognize /* fall through */ comments as
>>> implicit fall-through markings.
>>>
>>> Link: https://github.com/KSPP/linux/issues/115
>>> Signed-off-by: Gustavo A. R. Silva 
>>
>> It's not cool that you ignore the comments you got in [1], then after a
>> while mark the patch as "RESEND" and not even include a changelog why it
>> was resent.
>>
>> [1] 
>> https://patchwork.kernel.org/project/linux-wireless/patch/d522f387b2d0dde774785c7169c1f25aa529989d.1605896060.git.gustavo...@kernel.org/
> 
> Hm, this conversation looks like a miscommunication, mainly? I see
> Gustavo, as requested by many others[1], replacing the fallthrough
> comments with the "fallthrough" statement. (This is more than just a
> "Clang doesn't parse comments" issue.)
> 
> This could be a tree-wide patch and not bother you, but Greg KH has
> generally advised us to send these changes broken out. Anyway, this
> change still needs to land, so what would be the preferred path? I think
> Gustavo could just carry it for Linus to merge without bothering you if
> that'd be preferred?

I'll respond with the same I did last time, fallthrough is not C and
it's ugly.

Instead of mutilating the kernel, Gustavo should invest in fixing the
broken clang compiler.

Thanks,
Jes

Re: [PATCH 117/141] rtl8xxxu: Fix fall-through warnings for Clang

[Jes Sorensen]

On 11/20/20 1:38 PM, Gustavo A. R. Silva wrote:
> In preparation to enable -Wimplicit-fallthrough for Clang, fix
> multiple warnings by replacing /* fall through */ comments with
> the new pseudo-keyword macro fallthrough; instead of letting the
> code fall through to the next case.
> 
> Notice that Clang doesn't recognize /* fall through */ comments as
> implicit fall-through markings.
> 
> Link: https://github.com/KSPP/linux/issues/115
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

While I wasn't CC'ed on the cover-letter I see Jakub also raised issues
about this unnecessary patch noise.

Quite frankly, this seems to be patch churn for the sake of patch churn.
If clang is broken, fix clang instead.

NACK


Jes

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 5cd7ef3625c5..afc97958fa4d 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -1145,7 +1145,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 */
> + fallthrough;
>   case NL80211_CHAN_WIDTH_20:
>   opmode |= BW_OPMODE_20MHZ;
>   rtl8xxxu_write8(priv, REG_BW_OPMODE, opmode);
> @@ -1272,7 +1272,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 */
> + fallthrough;
>   case NL80211_CHAN_WIDTH_20:
>   rf_mode_bw |= WMAC_TRXPTCL_CTL_BW_20;
>   subchannel = 0;
> @@ -1741,11 +1741,11 @@ static int rtl8xxxu_identify_chip(struct 
> rtl8xxxu_priv *priv)
>   case 3:
>   priv->ep_tx_low_queue = 1;
>   priv->ep_tx_count++;
> - /* fall through */
> + fallthrough;
>   case 2:
>   priv->ep_tx_normal_queue = 1;
>   priv->ep_tx_count++;
> - /* fall through */
> + fallthrough;
>   case 1:
>   priv->ep_tx_high_queue = 1;
>   priv->ep_tx_count++;
> 

Re: [PATCH] rtl8xxxu: remove the unused variable timeout value assignment

[Jes Sorensen]

On 11/13/20 4:50 AM, xiakaixu1...@gmail.com wrote:
> From: Kaixu Xia 
> 
> The value of variable timeout is overwritten by the following statement in
> rtl8xxxu_gen1_init_aggregation(), so here the value assignment is useless.
> Remove it.
> 
> Reported-by: Tosk Robot 
> Signed-off-by: Kaixu Xia 
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 5cd7ef3..342126b 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -4426,7 +4426,7 @@ void rtl8xxxu_gen1_init_aggregation(struct 
> rtl8xxxu_priv *priv)
>   page_thresh = (priv->fops->rx_agg_buf_size / 512);
>   if (rtl8xxxu_dma_agg_pages >= 0) {
>   if (rtl8xxxu_dma_agg_pages <= page_thresh)
> - timeout = page_thresh;
> + ; /* do nothing */

Sorry this is the wrong way to do this. If the if statement is no longer
needed, then remove it, don't just make it do nothing.

Nack

Jes

Re: [RFC] Status of orinoco_usb

[Jes Sorensen]

On 10/6/20 3:45 AM, Arend Van Spriel wrote:
> + Jes
> 
> On 10/5/2020 4:12 PM, Kalle Valo wrote:
>> Greg Kroah-Hartman  writes:
>>
>>> On Fri, Oct 02, 2020 at 01:53:58PM +0200, Sebastian Andrzej Siewior
>>> wrote:
>>>> On 2020-10-02 13:37:25 [+0200], Greg Kroah-Hartman wrote:
>>>>>> Is it possible to end up here in softirq context or is this a relic?
>>>>>
>>>>> I think it's a relic of where USB host controllers completed their
>>>>> urbs
>>>>> in hard-irq mode.  The BH/tasklet change is a pretty recent change.
>>>>
>>>> But the BH thingy for HCDs went in v3.12 for EHCI. XHCI was v5.5. My
>>>> guess would be that people using orinoco USB are on EHCI :)
>>>
>>> USB 3 systems run XHCI, which has a USB 2 controller in it, so these
>>> types of things might not have been noticed yet.  Who knows :)
>>>
>>>>>> Should it be removed?
>>>>>
>>>>> We can move it out to drivers/staging/ and then drop it to see if
>>>>> anyone
>>>>> complains that they have the device and is willing to test any
>>>>> changes.
>>>>
>>>> Not sure moving is easy since it depends on other files in that folder.
>>>> USB is one interface next to PCI for instance. Unless you meant to move
>>>> the whole driver including all interfaces.
>>>> I was suggesting to remove the USB bits.
>>>
>>> I forgot this was tied into other code, sorry.  I don't know what to
>>> suggest other than maybe try to fix it up the best that you can, and
>>> let's see if anyone notices...
>>
>> That's what I would suggest as well.
>>
>> These drivers for ancient hardware are tricky. Even if there doesn't
>> seem to be any users on the driver sometimes people pop up reporting
>> that it's still usable. We had that recently with one another wireless
>> driver (forgot the name already).
> 
> Quite a while ago I shipped an orinoco dongle to Jes Sorensen which he
> wanted to use for some intern project if I recall correctly. Guess that
> idea did not fly yet.

I had an outreachy intern who worked on some of it, so I shipped all my
Orinoco hardware to her. We never made as much progress as I had hoped,
and I haven't had time to work on it since.

Cheers,
Jes

Re: [PATCH v16 3/3] Input: new da7280 haptic driver

[Jes Sorensen]

On 7/9/20 3:27 AM, Roy Im wrote:
> Adds support for the Dialog DA7280 LRA/ERM Haptic Driver with
> multiple mode and integrated waveform memory and wideband support.
> It communicates via an I2C bus to the device.
> 
> Signed-off-by: Roy Im 
> ---
> v16:
>   - Corrected some code and updated description in Kconfig.
> v15:
>   - Removed some defines and updated some comments.
> v14:
>   - Updated pwm related code, alignments and comments.
> v13:
>   - Updated some conditions in pwm function and alignments.
> v12: No changes.
> v11: 
>   - Updated the pwm related code, comments and typo.
> v10: 
>   - Updated the pwm related function and added some comments.
> v9: 
>   - Removed the header file and put the definitions into the c file.
>   - Updated the pwm code and error logs with %pE
> v8: 
>   - Added changes to support FF_PERIODIC/FF_CUSTOM and FF_CONSTANT.
>   - Updated the dt-related code.
>   - Removed memless related functions.
> v7: 
>   - Added more attributes to handle one value per file.
>   - Replaced and updated the dt-related code and functions called.
>   - Fixed error/functions.
> v6: No changes.
> v5: Fixed errors in Kconfig file.
> v4: Updated code as dt-bindings are changed.
> v3: No changes.
> v2: Fixed kbuild error/warning
> 
> 
>  drivers/input/misc/Kconfig  |   13 +
>  drivers/input/misc/Makefile |1 +
>  drivers/input/misc/da7280.c | 1840 
> +++
>  3 files changed, 1854 insertions(+)
>  create mode 100644 drivers/input/misc/da7280.c

Hi Roy,

Overall the driver looks pretty good now. I did find one issue, see
below. If you fix that I am happy to add a Reviewed-by line.

Reviewed-By: Jes Sorensen 

> diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
> new file mode 100644
> index 000..c8c42ac
> --- /dev/null
> +++ b/drivers/input/misc/da7280.c

[snip]

> +static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool enabled)
> +{
> + struct pwm_state state;
> + u64 period_mag_multi;
> + int error;
> +
> + if (!haptics->gain && enabled) {
> + dev_err(haptics->dev,
> + "Please set the gain first for the pwm mode\n");
> + return -EINVAL;
> + }
> +
> + pwm_get_state(haptics->pwm_dev, );
> + state.enabled = enabled;
> + if (enabled) {
> + period_mag_multi = state.period * haptics->gain;

You are multiplying an unsigned int to a u16 and storing it in a u64.
However, C doesn't promote the types, so you'll end up with an
unexpected result here. You can fix it by promoting state.period to u64, ie:

period_mage_multi = (u64)state.period * haptics->gain;

See the following example code which demonstrates the problem.

#include 
#include 

uint64_t foo(unsigned int a, uint16_t b)
{
uint64_t tmp = a * b;
return tmp;
}

uint64_t bar(unsigned int a, uint16_t b)
{
uint64_t tmp = (uint64_t)a * b;
return tmp;
}

int main()
{
uint64_t val;
unsigned int a = 0xff00ff00;
uint16_t b = 0x200;

val = foo(a, b);
printf("result(%0x, %0x) = %0llx\n", a, b, val);

val = bar(a, b);
printf("result(%0x, %0x) = %0llx\n", a, b, val);
}

Cheers,
Jes

Re: [PATCH v15 3/3] Input: new da7280 haptic driver

[Jes Sorensen]

On 6/29/20 9:01 AM, Roy Im wrote:
> Adds support for the Dialog DA7280 LRA/ERM Haptic Driver with
> multiple mode and integrated waveform memory and wideband support.
> It communicates via an I2C bus to the device.
> 
> Signed-off-by: Roy Im 
> ---
> v15:
>   - Removed some defines and updated some comments.
> v14:
>   - Updated pwm related code, alignments and comments.
> v13:
>   - Updated some conditions in pwm function and alignments.
> v12: No changes.
> v11: 
>   - Updated the pwm related code, comments and typo.
> v10: 
>   - Updated the pwm related function and added some comments.
> v9: 
>   - Removed the header file and put the definitions into the c file.
>   - Updated the pwm code and error logs with %pE
> v8: 
>   - Added changes to support FF_PERIODIC/FF_CUSTOM and FF_CONSTANT.
>   - Updated the dt-related code.
>   - Removed memless related functions.
> v7: 
>   - Added more attributes to handle one value per file.
>   - Replaced and updated the dt-related code and functions called.
>   - Fixed error/functions.
> v6: No changes.
> v5: Fixed errors in Kconfig file.
> v4: Updated code as dt-bindings are changed.
> v3: No changes.
> v2: Fixed kbuild error/warning
> 
> 
>  drivers/input/misc/Kconfig  |   13 +
>  drivers/input/misc/Makefile |1 +
>  drivers/input/misc/da7280.c | 1838 
> +++
>  3 files changed, 1852 insertions(+)
>  create mode 100644 drivers/input/misc/da7280.c

[snip]

> +static ssize_t
> +patterns_store(struct device *dev,
> +struct device_attribute *attr,
> +const char *buf,
> +size_t count)
> +{
> + struct da7280_haptic *haptics = dev_get_drvdata(dev);
> + char cmd[MAX_USER_INPUT_LEN];
> + struct parse_data_t mem;
> + unsigned int val;
> + int error;
> +
> + error = regmap_read(haptics->regmap, DA7280_MEM_CTL1, );
> + if (error)
> + return error;
> +
> + if (count > MAX_USER_INPUT_LEN)
> + memcpy(cmd, buf, MAX_USER_INPUT_LEN);
> + else
> + memcpy(cmd, buf, count);
> +
> + /* chop of '\n' introduced by echo at the end of the input */
> + if (cmd[count - 1] == '\n')
> + cmd[count - 1] = '\0';

You have a potential memory corruption bug here for the case where
 count > MAX_USER_INPUT_LEN. The code correctly clamps the memcpy()
length, but it still is at risk of writing beyond the end of the cmd
buffer when doing the \0 termination.

If you change the code above to say

if (count > MAX_USER_INPUT_LEN)
count = MAX_USER_INPUT_LEN
memcpy(cmd, buf, count);

it should take care of it, and it will also return the actual count
written to the caller.

Cheers,
Jes

Re: [PATCH] rtl8xxxu: fix RTL8723BU connection failure issue after warm reboot

[Jes Sorensen]

On 10/15/19 6:19 AM, Chris Chiu wrote:

The RTL8723BU has problems connecting to AP after each warm reboot.
Sometimes it returns no scan result, and in most cases, it fails
the authentication for unknown reason. However, it works totally
fine after cold reboot.

Compare the value of register SYS_CR and SYS_CLK_MAC_CLK_ENABLE
for cold reboot and warm reboot, the registers imply that the MAC
is already powered and thus some procedures are skipped during
driver initialization. Double checked the vendor driver, it reads
the SYS_CR and SYS_CLK_MAC_CLK_ENABLE also but doesn't skip any
during initialization based on them. This commit only tells the
RTL8723BU to do full initilization without checking MAC status.

Signed-off-by: Chris Chiu 
---
  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h   | 1 +
  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 1 +
  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c  | 3 +++
  3 files changed, 5 insertions(+)


Looks good to me! If this takes care of the warm boot problem, that's 
pretty awesome.


Signed-off-by: Jes Sorensen 

Jes

Re: [PATCH] rtl8xxxu: make arrays static, makes object smaller

[Jes Sorensen]

On 10/7/19 9:53 AM, Colin King wrote:

From: Colin Ian King 

Don't populate const arrays on the stack but instead make them
static. Makes the object code smaller by 60 bytes.

Before:
text   data bss dec hex filename
   15133   8768   0   239015d5d 
realtek/rtl8xxxu/rtl8xxxu_8192e.o
   15209   6392   0   216015461 
realtek/rtl8xxxu/rtl8xxxu_8723b.o
  103254  31202 576  135032   20f78 realtek/rtl8xxxu/rtl8xxxu_core.o

After:
text   data bss dec hex filename
   14861   9024   0   238855d4d 
realtek/rtl8xxxu/rtl8xxxu_8192e.o
   14953   6616   0   215695441 
realtek/rtl8xxxu/rtl8xxxu_8723b.o
  102986  31458 576  135020   20f6c realtek/rtl8xxxu/rtl8xxxu_core.o

(gcc version 9.2.1, amd64)

Signed-off-by: Colin Ian King 


Looks fine to me!

Assume you mean x86_64 since there's no such thing as an amd64 
architecture :)


Cheers,
Jes

Re: [PATCH v3] rtl8xxxu: add bluetooth co-existence support for single antenna

[Jes Sorensen]

On 10/5/19 5:48 AM, Chris Chiu wrote:

The RTL8723BU suffers the wifi disconnection problem while bluetooth
device connected. While wifi is doing tx/rx, the bluetooth will scan
without results. This is due to the wifi and bluetooth share the same
single antenna for RF communication and they need to have a mechanism
to collaborate.

BT information is provided via the packet sent from co-processor to
host (C2H). It contains the status of BT but the rtl8723bu_handle_c2h
dose not really handle it. And there's no bluetooth coexistence
mechanism to deal with it.

This commit adds a workqueue to set the tdma configurations and
coefficient table per the parsed bluetooth link status and given
wifi connection state. The tdma/coef table comes from the vendor
driver code of the RTL8192EU and RTL8723BU. However, this commit is
only for single antenna scenario which RTL8192EU is default dual
antenna. The rtl8xxxu_parse_rxdesc24 which invokes the handle_c2h
is only for 8723b and 8192e so the mechanism is expected to work
on both chips with single antenna. Note RTL8192EU dual antenna is
not supported.

Signed-off-by: Chris Chiu 
---

Notes:
   v2:
- Add helper functions to replace bunch of tdma settings
- Reformat some lines to meet kernel coding style
   v3:
- Add comment for rtl8723bu_set_coex_with_type()


Looks good to me!

Signed-off-by: Jes Sorensen 

Jes

Re: [PATCH v2] rtl8xxxu: add bluetooth co-existence support for single antenna

[Jes Sorensen]

On 10/2/19 9:19 PM, Chris Chiu wrote:

On Wed, Oct 2, 2019 at 11:04 PM Jes Sorensen  wrote:



In general I think it looks good! One nit below:

Sorry I have been traveling for the last three weeks, so just catching up.



+void rtl8723bu_set_coex_with_type(struct rtl8xxxu_priv *priv, u8 type)
+{
+ switch (type) {
+ case 0:
+ rtl8xxxu_write32(priv, REG_BT_COEX_TABLE1, 0x);
+ rtl8xxxu_write32(priv, REG_BT_COEX_TABLE2, 0x);
+ rtl8xxxu_write32(priv, REG_BT_COEX_TABLE3, 0x00ff);
+ rtl8xxxu_write8(priv, REG_BT_COEX_TABLE4, 0x03);
+ break;
+ case 1:
+ case 3:


The one item here, I would prefer introducing some defined types to
avoid the hard coded type numbers. It's much easier to read and debug
when named.


Honestly, I also thought of that but there's no meaningful description for these
numbers in the vendor driver. Even based on where they're invoked, I can merely
give a rough definition on 0. So I left it as it is for the covenience
if I have to do
cross-comparison with vendor driver in the future for some possible
unknown bugs.


If you shortened the name of the function to rtl8723bu_set_coex() you
won't have problems with line lengths at the calling point.


I think the rtl8723bu_set_ps_tdma() function would cause the line length problem
more than rtl8723bu_set_coex_with_type() at the calling point. But as the same
debug reason as mentioned, I may like to keep it because I don't know how to
categorize the 5 magic parameters. I also reference the latest rtw88
driver code,
it seems no better solution so far. I'll keep watching if there's any
better idea.


Personally I would still prefer to name it COEX_TYPE_1 etc. but I can 
live with this. Would you mind at least adding some comments in the code 
about it?


Cheers,
Jes

Re: [PATCH v2] rtl8xxxu: add bluetooth co-existence support for single antenna

[Jes Sorensen]

On 9/10/19 10:50 PM, Chris Chiu wrote:

The RTL8723BU suffers the wifi disconnection problem while bluetooth
device connected. While wifi is doing tx/rx, the bluetooth will scan
without results. This is due to the wifi and bluetooth share the same
single antenna for RF communication and they need to have a mechanism
to collaborate.

BT information is provided via the packet sent from co-processor to
host (C2H). It contains the status of BT but the rtl8723bu_handle_c2h
dose not really handle it. And there's no bluetooth coexistence
mechanism to deal with it.

This commit adds a workqueue to set the tdma configurations and
coefficient table per the parsed bluetooth link status and given
wifi connection state. The tdma/coef table comes from the vendor
driver code of the RTL8192EU and RTL8723BU. However, this commit is
only for single antenna scenario which RTL8192EU is default dual
antenna. The rtl8xxxu_parse_rxdesc24 which invokes the handle_c2h
is only for 8723b and 8192e so the mechanism is expected to work
on both chips with single antenna. Note RTL8192EU dual antenna is
not supported.

Signed-off-by: Chris Chiu 
---

Notes:
   v2:
- Add helper functions to replace bunch of tdma settings
- Reformat some lines to meet kernel coding style


  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  37 +++
  .../realtek/rtl8xxxu/rtl8xxxu_8723b.c |   2 -
  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 262 +-
  3 files changed, 294 insertions(+), 7 deletions(-)


In general I think it looks good! One nit below:

Sorry I have been traveling for the last three weeks, so just catching up.



+void rtl8723bu_set_coex_with_type(struct rtl8xxxu_priv *priv, u8 type)
+{
+   switch (type) {
+   case 0:
+   rtl8xxxu_write32(priv, REG_BT_COEX_TABLE1, 0x);
+   rtl8xxxu_write32(priv, REG_BT_COEX_TABLE2, 0x);
+   rtl8xxxu_write32(priv, REG_BT_COEX_TABLE3, 0x00ff);
+   rtl8xxxu_write8(priv, REG_BT_COEX_TABLE4, 0x03);
+   break;
+   case 1:
+   case 3:


The one item here, I would prefer introducing some defined types to 
avoid the hard coded type numbers. It's much easier to read and debug 
when named.


If you shortened the name of the function to rtl8723bu_set_coex() you 
won't have problems with line lengths at the calling point.


Cheers,
Jes

Re: [PATCH v7] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver

[Jes Sorensen]

On 9/17/19 3:40 AM, Chris Chiu wrote:
> We have 3 laptops which connect the wifi by the same RTL8723BU.
> The PCI VID/PID of the wifi chip is 10EC:B720 which is supported.
> They have the same problem with the in-kernel rtl8xxxu driver, the
> iperf (as a client to an ethernet-connected server) gets ~1Mbps.
> Nevertheless, the signal strength is reported as around -40dBm,
> which is quite good. From the wireshark capture, the tx rate for each
> data and qos data packet is only 1Mbps. Compare to the Realtek driver
> at https://github.com/lwfinger/rtl8723bu, the same iperf test gets
> ~12Mbps or better. The signal strength is reported similarly around
> -40dBm. That's why we want to improve.
> 
> After reading the source code of the rtl8xxxu driver and Realtek's, the
> major difference is that Realtek's driver has a watchdog which will keep
> monitoring the signal quality and updating the rate mask just like the
> rtl8xxxu_gen2_update_rate_mask() does if signal quality changes.
> And this kind of watchdog also exists in rtlwifi driver of some specific
> chips, ex rtl8192ee, rtl8188ee, rtl8723ae, rtl8821ae...etc. They have
> the same member function named dm_watchdog and will invoke the
> corresponding dm_refresh_rate_adaptive_mask to adjust the tx rate
> mask.
> 
> With this commit, the tx rate of each data and qos data packet will
> be 39Mbps (MCS4) with the 0xF0 as the tx rate mask. The 20th bit
> to 23th bit means MCS4 to MCS7. It means that the firmware still picks
> the lowest rate from the rate mask and explains why the tx rate of
> data and qos data is always lowest 1Mbps because the default rate mask
> passed is always 0xFFF ranges from the basic CCK rate, OFDM rate,
> and MCS rate. However, with Realtek's driver, the tx rate observed from
> wireshark under the same condition is almost 65Mbps or 72Mbps, which
> indicating that rtl8xxxu could still be further improved.
> 
> Signed-off-by: Chris Chiu 
> Reviewed-by: Daniel Drake 

I am still traveling after Plumbers and don't have my 8723bu dongles
with me, but I'd say this looks good.

Acked-by: Jes Sorensen 

Jes

Re: [Openipmi-developer] [PATCH 0/1] Fix race in ipmi timer cleanup

[Jes Sorensen]

On 9/14/19 9:08 PM, Corey Minyard wrote:
>>
>>>
>>> {disable,enable}_si_irq() themselves are racy:
>>>
>>> static inline bool disable_si_irq(struct smi_info *smi_info)
>>> {
>>> if ((smi_info->io.irq) && (!smi_info->interrupt_disabled)) {
>>> smi_info->interrupt_disabled = true;
>>>
>>> Basically interrupt_disabled need to be atomic here to have any value,
>>> unless you ensure to have a spin lock around every access to it.
>>
>> It needs to be atomic, yes, but I think just adding the spinlock like
>> I suggested will work.  You are right, the check for timer_running is
>> not necessary here, and I'm fine with removing it, but there are other
>> issues with interrupt_disabled (as you said) and with memory ordering
>> in the timer case.  So even if you remove the timer running check, the
>> lock is still required here.
> 
> It turns out you were right, all that really needs to be done is the
> del_timer_sync().  I've added your patch to my queue.
> 
> Sorry for the trouble.

Awesome!

Sorry I was going to get back and look at it again, but Linux Plumbers
and playing sardine i a tin can got in the way.

Cheers,
Jes

Re: [PATCH] rtl8xxxu: add bluetooth co-existence support for single antenna

[Jes Sorensen]

On 9/3/19 1:37 AM, Chris Chiu wrote:
> The RTL8723BU suffers the wifi disconnection problem while bluetooth
> device connected. While wifi is doing tx/rx, the bluetooth will scan
> without results. This is due to the wifi and bluetooth share the same
> single antenna for RF communication and they need to have a mechanism
> to collaborate.
> 
> BT information is provided via the packet sent from co-processor to
> host (C2H). It contains the status of BT but the rtl8723bu_handle_c2h
> dose not really handle it. And there's no bluetooth coexistence
> mechanism to deal with it.
> 
> This commit adds a workqueue to set the tdma configurations and
> coefficient table per the parsed bluetooth link status and given
> wifi connection state. The tdma/coef table comes from the vendor
> driver code of the RTL8192EU and RTL8723BU. However, this commit is
> only for single antenna scenario which RTL8192EU is default dual
> antenna. The rtl8xxxu_parse_rxdesc24 which invokes the handle_c2h
> is only for 8723b and 8192e so the mechanism is expected to work
> on both chips with single antenna. Note RTL8192EU dual antenna is
> not supported.

I am pretty excited to see this! It always bugged me the bluetooth
driver was allowed to be applied breaking the existing wifi driver.

Except for some cosmetic stuff, I am all happy with this.

> Signed-off-by: Chris Chiu 
> ---
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  37 +++
>  .../realtek/rtl8xxxu/rtl8xxxu_8723b.c |   2 -
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 243 +-
>  3 files changed, 275 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index 582c2a346cec..22e95b11bfbb 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h

> +
> +struct rtl8xxxu_btcoex {
> + u8  bt_status;
> + boolbt_busy;
> + boolhas_sco;
> + boolhas_a2dp;
> + boolhas_hid;
> + boolhas_pan;
> + boolhid_only;
> + boola2dp_only;
> + boolc2h_bt_inquiry;
> +};

bool is large, maybe just use flags or u8's for this? Not a big deal though.

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index a6f358b9e447..4f72c2d14d44 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c


> + if (!btcoex->has_a2dp &&
> + !btcoex->has_sco &&
> + !btcoex->has_pan &&
> + btcoex->has_hid)

This should all fit in one line - 80 characters

> + btcoex->hid_only = true;
> + else
> + btcoex->hid_only = false;
> +
> + if (!btcoex->has_sco &&
> + !btcoex->has_pan &&
> + !btcoex->has_hid &&
> + btcoex->has_a2dp)

Ditto

> +static void rtl8xxxu_c2hcmd_callback(struct work_struct *work)
> +{
> + struct rtl8xxxu_priv *priv;
> + struct rtl8723bu_c2h *c2h;
> + struct ieee80211_vif *vif;
> + struct device *dev;
> + struct sk_buff *skb = NULL;
> + unsigned long flags;
> + int len;
> + u8 bt_info = 0;
> + struct rtl8xxxu_btcoex *btcoex;
> +
> + priv = container_of(work, struct rtl8xxxu_priv, c2hcmd_work);
> + vif = priv->vif;
> + btcoex = >bt_coex;
> + dev = >udev->dev;
> +
> + if (priv->rf_paths > 1)
> + goto out;
> +
> + while (!skb_queue_empty(>c2hcmd_queue)) {
> + spin_lock_irqsave(>c2hcmd_lock, flags);
> + skb = __skb_dequeue(>c2hcmd_queue);
> + spin_unlock_irqrestore(>c2hcmd_lock, flags);
> +
> + c2h = (struct rtl8723bu_c2h *)skb->data;
> + len = skb->len - 2;
> +
> + switch (c2h->id) {
> + case C2H_8723B_BT_INFO:
> + bt_info = c2h->bt_info.bt_info;
> +
> + rtl8723bu_update_bt_link_info(priv, bt_info);
> +
> + if (btcoex->c2h_bt_inquiry) {
> + if (vif && !vif->bss_conf.assoc) {
> + rtl8723bu_set_ps_tdma(priv, 0x8, 0x0, 
> 0x0, 0x0, 0x0);
> + rtl8723bu_set_coex_with_type(priv, 0);
> + } else if (btcoex->has_sco ||
> +btcoex->has_hid ||
> +btcoex->has_a2dp) {
> + rtl8723bu_set_ps_tdma(priv, 0x61, 0x35, 
> 0x3, 0x11, 0x11);
> + rtl8723bu_set_coex_with_type(priv, 4);
> + } else if (btcoex->has_pan) {
> + rtl8723bu_set_ps_tdma(priv, 0x61, 0x3f, 
> 0x3, 0x11, 0x11);
> + rtl8723bu_set_coex_with_type(priv, 4);
> + } else 

Re: [PATCH 0/1] Fix race in ipmi timer cleanup

[Jes Sorensen]

On 8/28/19 6:32 PM, Corey Minyard wrote:
> On Wed, Aug 28, 2019 at 04:36:24PM -0400, Jes Sorensen wrote:
>> From: Jes Sorensen 
>>
>> I came across this in 4.16, but I believe the bug is still present
>> in current 5.x, even if it is less likely to trigger.
>>
>> Basially stop_timer_and_thread() only calls del_timer_sync() if
>> timer_running == true. However smi_mod_timer enables the timer before
>> setting timer_running = true.
> 
> All the modifications/checks for timer_running should be done under
> the si_lock.  It looks like a lock is missing in shutdown_smi(),
> probably starting before setting interrupt_disabled to true and
> after stop_timer_and_thread.  I think that is the right fix for
> this problem.

Hi Corey,

I agree a spin lock could deal with this specific issue too, but calling
del_timer_sync() is safe to call on an already disabled timer. The whole
flagging of timer_running really doesn't make much sense in the first
place either.

As for interrupt_disabled that is even worse. There's multiple places in
the code where interrupt_disabled is checked, some of them are not
protected by a spin lock, including shutdown_smi() where you have this
sequence:

while (smi_info->curr_msg || (smi_info->si_state != SI_NORMAL)){
poll(smi_info);
schedule_timeout_uninterruptible(1);
}
if (smi_info->handlers)
disable_si_irq(smi_info);
while (smi_info->curr_msg || (smi_info->si_state != SI_NORMAL)){
poll(smi_info);
schedule_timeout_uninterruptible(1);
}

In this case you'll have to drop and retake the long several times.

You also have this call sequence which leads to disable_si_irq() which
checks interrupt_disabled:

  flush_messages()
smi_event_handler()
  handle_transaction_done()
handle_flags()
  alloc_msg_handle_irq()
disable_si_irq()

{disable,enable}_si_irq() themselves are racy:

static inline bool disable_si_irq(struct smi_info *smi_info)
{
if ((smi_info->io.irq) && (!smi_info->interrupt_disabled)) {
smi_info->interrupt_disabled = true;

Basically interrupt_disabled need to be atomic here to have any value,
unless you ensure to have a spin lock around every access to it.

Cheers,
Jes

[PATCH 1/1] ipmi_si_intf: Fix race in timer shutdown handling

[Jes Sorensen]

From: Jes Sorensen 

smi_mod_timer() enables the timer before setting timer_running. This
means the timer can be running when we get to stop_timer_and_thread()
without timer_running having been set, resulting in del_timer_sync()
not being called and the timer being left to cause havoc during
shutdown.

Instead just call del_timer_sync() unconditionally

Signed-off-by: Jes Sorensen 
---
 drivers/char/ipmi/ipmi_si_intf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index da5b6723329a..53425e25ecf4 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1828,8 +1828,7 @@ static inline void stop_timer_and_thread(struct smi_info 
*smi_info)
}
 
smi_info->timer_can_start = false;
-   if (smi_info->timer_running)
-   del_timer_sync(_info->si_timer);
+   del_timer_sync(_info->si_timer);
 }
 
 static struct smi_info *find_dup_si(struct smi_info *info)
-- 
2.21.0

[PATCH 0/1] Fix race in ipmi timer cleanup

[Jes Sorensen]

From: Jes Sorensen 

I came across this in 4.16, but I believe the bug is still present
in current 5.x, even if it is less likely to trigger.

Basially stop_timer_and_thread() only calls del_timer_sync() if
timer_running == true. However smi_mod_timer enables the timer before
setting timer_running = true.

I was able to reproduce this in 4.16 running the following on a host

   while :; do rmmod ipmi_si ; modprobe ipmi_si; done

while rebooting the BMC on it in parallel.

5.2 moves the error handling around and does it more centralized, but
relying on timer_running still seems dubious to me.

static void smi_mod_timer(struct smi_info *smi_info, unsigned long new_val)
{
if (!smi_info->timer_can_start)
return;
smi_info->last_timeout_jiffies = jiffies;
mod_timer(_info->si_timer, new_val);
smi_info->timer_running = true;
}

static inline void stop_timer_and_thread(struct smi_info *smi_info)
{
if (smi_info->thread != NULL) {
kthread_stop(smi_info->thread);
smi_info->thread = NULL;
}

smi_info->timer_can_start = false;
if (smi_info->timer_running)
del_timer_sync(_info->si_timer);
}

Cheers,
Jes

Jes Sorensen (1):
  ipmi_si_intf: Fix race in timer shutdown handling

 drivers/char/ipmi/ipmi_si_intf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.21.0

Re: [RFC PATCH v7] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver

[Jes Sorensen]

On 8/12/19 10:32 AM, Kalle Valo wrote:
> Jes Sorensen  writes:
>> Looks good to me! Nice work! I am actually very curious if this will
>> improve performance 8192eu as well.
>>
>> Ideally I'd like to figure out how to make host controlled rates work,
>> but in all my experiments with that, I never really got it to work well.
>>
>> Signed-off-by: Jes Sorensen 
> 
> This is marked as RFC so I'm not sure what's the plan. Should I apply
> this?

I think it's at a point where it's worth applying - I kinda wish I had
had time to test it, but I won't be near my stash of USB dongles for a
little while.

Cheers,
Jes

Re: [RFC PATCH v7] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver

[Jes Sorensen]

On 8/5/19 9:14 AM, Chris Chiu wrote:
> We have 3 laptops which connect the wifi by the same RTL8723BU.
> The PCI VID/PID of the wifi chip is 10EC:B720 which is supported.
> They have the same problem with the in-kernel rtl8xxxu driver, the
> iperf (as a client to an ethernet-connected server) gets ~1Mbps.
> Nevertheless, the signal strength is reported as around -40dBm,
> which is quite good. From the wireshark capture, the tx rate for each
> data and qos data packet is only 1Mbps. Compare to the Realtek driver
> at https://github.com/lwfinger/rtl8723bu, the same iperf test gets
> ~12Mbps or better. The signal strength is reported similarly around
> -40dBm. That's why we want to improve.
> 
> After reading the source code of the rtl8xxxu driver and Realtek's, the
> major difference is that Realtek's driver has a watchdog which will keep
> monitoring the signal quality and updating the rate mask just like the
> rtl8xxxu_gen2_update_rate_mask() does if signal quality changes.
> And this kind of watchdog also exists in rtlwifi driver of some specific
> chips, ex rtl8192ee, rtl8188ee, rtl8723ae, rtl8821ae...etc. They have
> the same member function named dm_watchdog and will invoke the
> corresponding dm_refresh_rate_adaptive_mask to adjust the tx rate
> mask.
> 
> With this commit, the tx rate of each data and qos data packet will
> be 39Mbps (MCS4) with the 0xF0 as the tx rate mask. The 20th bit
> to 23th bit means MCS4 to MCS7. It means that the firmware still picks
> the lowest rate from the rate mask and explains why the tx rate of
> data and qos data is always lowest 1Mbps because the default rate mask
> passed is always 0xFFF ranges from the basic CCK rate, OFDM rate,
> and MCS rate. However, with Realtek's driver, the tx rate observed from
> wireshark under the same condition is almost 65Mbps or 72Mbps, which
> indicating that rtl8xxxu could still be further improved.
> 
> Signed-off-by: Chris Chiu 
> Reviewed-by: Daniel Drake 
> ---

Looks good to me! Nice work! I am actually very curious if this will
improve performance 8192eu as well.

Ideally I'd like to figure out how to make host controlled rates work,
but in all my experiments with that, I never really got it to work well.

Signed-off-by: Jes Sorensen 

Jes


> 
> Notes:
>   v2:
>- Fix errors and warnings complained by checkpatch.pl
>- Replace data structure rate_adaptive by 2 member variables
>- Make rtl8xxxu_wireless_mode non-static
>- Runs refresh_rate_mask() only in station mode
>   v3:
>- Remove ugly rtl8xxxu_watchdog data structure
>- Make sure only one vif exists
>   v4:
>- Move cancel_delayed_work from rtl8xxxu_disconnect to rtl8xxxu_stop
>- Clear priv->vif in rtl8xxxu_remove_interface
>- Add rateid as the function argument of update_rate_mask
>- Rephrase the comment for priv->vif more explicit.
>   v5:
>- Make refresh_rate_mask() generic for all sub-drivers.
>- Add definitions for SNR related to help determine rssi_level
>   v6: 
>- Fix typo of the comment for priv->vif
>   v7:
>- Fix reported bug of watchdog stop 
>- refer to the RxPWDBAll in vendor driver for SNR calculation
> 
> 
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  55 -
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 229 +-
>  2 files changed, 277 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index ade057d868f7..582c2a346cec 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -1187,6 +1187,48 @@ struct rtl8723bu_c2h {
>  
>  struct rtl8xxxu_fileops;
>  
> +/*mlme related.*/
> +enum wireless_mode {
> + WIRELESS_MODE_UNKNOWN = 0,
> + /* Sub-Element */
> + WIRELESS_MODE_B = BIT(0),
> + WIRELESS_MODE_G = BIT(1),
> + WIRELESS_MODE_A = BIT(2),
> + WIRELESS_MODE_N_24G = BIT(3),
> + WIRELESS_MODE_N_5G = BIT(4),
> + WIRELESS_AUTO = BIT(5),
> + WIRELESS_MODE_AC = BIT(6),
> + WIRELESS_MODE_MAX = 0x7F,
> +};
> +
> +/* from rtlwifi/wifi.h */
> +enum ratr_table_mode_new {
> + RATEID_IDX_BGN_40M_2SS = 0,
> + RATEID_IDX_BGN_40M_1SS = 1,
> + RATEID_IDX_BGN_20M_2SS_BN = 2,
> + RATEID_IDX_BGN_20M_1SS_BN = 3,
> + RATEID_IDX_GN_N2SS = 4,
> + RATEID_IDX_GN_N1SS = 5,
> + RATEID_IDX_BG = 6,
> + RATEID_IDX_G = 7,
> + RATEID_IDX_B = 8,
> + RATEID_IDX_VHT_2SS = 9,
> + RATEID_IDX_VHT_1SS = 10,
> + RATEID_IDX_MIX1 = 11,
> + RATEID_IDX_MIX2 = 12,
> + RATEID_IDX_VHT_3SS = 13,
> + RATEID_IDX_BGN_3SS = 14,
> +};
> +

Re: [PATCH] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU

[Jes Sorensen]

On 7/4/19 11:44 PM, Daniel Drake wrote:
> On Wed, Jul 3, 2019 at 8:59 PM Jes Sorensen  wrote:
>> My point is this seems to be very dongle dependent :( We have to be
>> careful not breaking it for some users while fixing it for others.
> 
> Do you still have your device?
> 
> Once we get to the point when you are happy with Chris's two patches
> here on a code review level, we'll reach out to other driver
> contributors plus people who previously complained about these types
> of problems, and see if we can get some wider testing.

I should have them, but I won't have access to them for another 2.5
weeks unfortunately.

Cheers,
Jes

Re: [PATCH v2] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU

[Jes Sorensen]

On 7/4/19 10:27 PM, Chris Chiu wrote:
> On Fri, Jul 5, 2019 at 12:43 AM Jes Sorensen  wrote:
>>
>> On 7/4/19 6:55 AM, Chris Chiu wrote:
>>> The WiFi tx power of RTL8723BU is extremely low after booting. So
>>> the WiFi scan gives very limited AP list and it always fails to
>>> connect to the selected AP. This module only supports 1x1 antenna
>>> and the antenna is switched to bluetooth due to some incorrect
>>> register settings.
>>>
>>> Compare with the vendor driver https://github.com/lwfinger/rtl8723bu,
>>> we realized that the 8723bu's enable_rf() does the same thing as
>>> rtw_btcoex_HAL_Initialize() in vendor driver. And it by default
>>> sets the antenna path to BTC_ANT_PATH_BT which we verified it's
>>> the cause of the wifi weak tx power. The vendor driver will set
>>> the antenna path to BTC_ANT_PATH_PTA in the consequent btcoexist
>>> mechanism, by the function halbtc8723b1ant_PsTdma.
>>>
>>> This commit hand over the antenna control to PTA(Packet Traffic
>>> Arbitration), which compares the weight of bluetooth/wifi traffic
>>> then determine whether to continue current wifi traffic or not.
>>> After PTA take control, The wifi signal will be back to normal and
>>> the bluetooth scan can also work at the same time. However, the
>>> btcoexist still needs to be handled under different circumstances.
>>> If there's a BT connection established, the wifi still fails to
>>> connect until BT disconnected.
>>>
>>> Signed-off-by: Chris Chiu 
>>> ---
>>>
>>>
>>> Note:
>>>  v2:
>>>   - Replace BIT(11) with the descriptive definition
>>>   - Meaningful comment for the REG_S0S1_PATH_SWITCH setting
>>>
>>>
>>>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 11 ---
>>>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c  |  3 ++-
>>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c 
>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
>>> index 3adb1d3d47ac..ceffe05bd65b 100644
>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
>>> @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv 
>>> *priv)
>>>   /*
>>>* WLAN action by PTA
>>>*/
>>> - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04);
>>> + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c);
>>>
>>>   /*
>>>* BT select S0/S1 controlled by WiFi
>>> @@ -1568,9 +1568,14 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv 
>>> *priv)
>>>   rtl8xxxu_gen2_h2c_cmd(priv, , sizeof(h2c.ant_sel_rsv));
>>>
>>>   /*
>>> -  * 0x280, 0x00, 0x200, 0x80 - not clear
>>> +  * Different settings per different antenna position.
>>> +  *  Antenna Position:   | Normal   Inverse
>>> +  * --
>>> +  * Antenna switch to BT:|  0x280,   0x00
>>> +  * Antenna switch to WiFi:  |  0x0, 0x280
>>> +  * Antenna switch to PTA:   |  0x200,   0x80
>>>*/
>>> - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00);
>>> + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80);
>>
>> Per the documentation, shouldn't this be set to 0x200 then rather than 0x80?
>>
> Per the code before REG_S0S1_PATH_SWITCH setting, the driver has told
> the co-processor the antenna is inverse.
> memset(, 0, sizeof(struct h2c_cmd));
> h2c.ant_sel_rsv.cmd = H2C_8723B_ANT_SEL_RSV;
> h2c.ant_sel_rsv.ant_inverse = 1;
> h2c.ant_sel_rsv.int_switch_type = 0;
> rtl8xxxu_gen2_h2c_cmd(priv, , sizeof(h2c.ant_sel_rsv));
> 
> At least the current modification is consistent with the antenna
> inverse setting.
> I'll verify on vendor driver about when/how the inverse be determined.

Fair enough :)

Cheers,
Jes

Re: [PATCH v2] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU

[Jes Sorensen]

On 7/4/19 6:55 AM, Chris Chiu wrote:
> The WiFi tx power of RTL8723BU is extremely low after booting. So
> the WiFi scan gives very limited AP list and it always fails to
> connect to the selected AP. This module only supports 1x1 antenna
> and the antenna is switched to bluetooth due to some incorrect
> register settings.
> 
> Compare with the vendor driver https://github.com/lwfinger/rtl8723bu,
> we realized that the 8723bu's enable_rf() does the same thing as
> rtw_btcoex_HAL_Initialize() in vendor driver. And it by default
> sets the antenna path to BTC_ANT_PATH_BT which we verified it's
> the cause of the wifi weak tx power. The vendor driver will set
> the antenna path to BTC_ANT_PATH_PTA in the consequent btcoexist
> mechanism, by the function halbtc8723b1ant_PsTdma.
> 
> This commit hand over the antenna control to PTA(Packet Traffic
> Arbitration), which compares the weight of bluetooth/wifi traffic
> then determine whether to continue current wifi traffic or not.
> After PTA take control, The wifi signal will be back to normal and
> the bluetooth scan can also work at the same time. However, the
> btcoexist still needs to be handled under different circumstances.
> If there's a BT connection established, the wifi still fails to
> connect until BT disconnected.
> 
> Signed-off-by: Chris Chiu 
> ---
> 
> 
> Note:
>  v2:
>   - Replace BIT(11) with the descriptive definition
>   - Meaningful comment for the REG_S0S1_PATH_SWITCH setting
> 
> 
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 11 ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c  |  3 ++-
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> index 3adb1d3d47ac..ceffe05bd65b 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv 
> *priv)
>   /*
>* WLAN action by PTA
>*/
> - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04);
> + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c);
>  
>   /*
>* BT select S0/S1 controlled by WiFi
> @@ -1568,9 +1568,14 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv 
> *priv)
>   rtl8xxxu_gen2_h2c_cmd(priv, , sizeof(h2c.ant_sel_rsv));
>  
>   /*
> -  * 0x280, 0x00, 0x200, 0x80 - not clear
> +  * Different settings per different antenna position.
> +  *  Antenna Position:   | Normal   Inverse
> +  * --
> +  * Antenna switch to BT:|  0x280,   0x00
> +  * Antenna switch to WiFi:  |  0x0, 0x280
> +  * Antenna switch to PTA:   |  0x200,   0x80
>*/
> - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00);
> + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80);

Per the documentation, shouldn't this be set to 0x200 then rather than 0x80?

We may need to put in place some code to detect whether we have normal
or inverse configuration of the dongle otherwise?

I really appreciate you're digging into this!

Cheers,
Jes

Re: [PATCH] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU

[Jes Sorensen]

On 7/2/19 11:25 PM, Chris Chiu wrote:
> On Tue, Jul 2, 2019 at 8:44 PM Jes Sorensen  wrote:
>>
>> On 6/27/19 5:52 AM, Chris Chiu wrote:
>>> The WiFi tx power of RTL8723BU is extremely low after booting. So
>>> the WiFi scan gives very limited AP list and it always fails to
>>> connect to the selected AP. This module only supports 1x1 antenna
>>> and the antenna is switched to bluetooth due to some incorrect
>>> register settings.
>>>
>>> This commit hand over the antenna control to PTA, the wifi signal
>>> will be back to normal and the bluetooth scan can also work at the
>>> same time. However, the btcoexist still needs to be handled under
>>> different circumstances. If there's a BT connection established,
>>> the wifi still fails to connect until disconneting the BT.
>>>
>>> Signed-off-by: Chris Chiu 
>>> ---
>>>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 9 ++---
>>>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c  | 3 ++-
>>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c 
>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
>>> index 3adb1d3d47ac..6c3c70d93ac1 100644
>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
>>> @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv 
>>> *priv)
>>>   /*
>>>* WLAN action by PTA
>>>*/
>>> - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04);
>>> + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c);
>>>
>>>   /*
>>>* BT select S0/S1 controlled by WiFi
>>> @@ -1568,9 +1568,12 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv 
>>> *priv)
>>>   rtl8xxxu_gen2_h2c_cmd(priv, , sizeof(h2c.ant_sel_rsv));
>>>
>>>   /*
>>> -  * 0x280, 0x00, 0x200, 0x80 - not clear
>>> +  * Different settings per different antenna position.
>>> +  * Antenna switch to BT: 0x280, 0x00 (inverse)
>>> +  * Antenna switch to WiFi: 0x0, 0x280 (inverse)
>>> +  * Antenna controlled by PTA: 0x200, 0x80 (inverse)
>>>*/
>>> - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00);
>>> + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80);
>>>
>>>   /*
>>>* Software control, antenna at WiFi side
>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> index 8136e268b4e6..87b2179a769e 100644
>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw 
>>> *hw)
>>>
>>>   /* Check if MAC is already powered on */
>>>   val8 = rtl8xxxu_read8(priv, REG_CR);
>>> + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR);
>>>
>>>   /*
>>>* 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 || !(val16 & BIT(11)))
>>>   macpower = false;
>>>   else
>>>   macpower = true;
>>
>> This part I would like to ask you take a good look at the other chips to
>> make sure you don't break support for 8192cu, 8723au, 8188eu with this.
>>
>> Cheers,
>> Jes
> 
> I checked the vendor code of 8192cu and 8188eu, they don't have this part
> of code to check the REG_CR before power on sequence. I can only find
> similar code in rtl8723be.
> if (tmp_u1b != 0 && tmp_u1b !=0xea)
> rtlhal->mac_func_enable = true;
> 
> By definition, the BIT(11) of REG_SYS_CLKR in rtl8xxxu_regs.h is
> SYS_CLK_MAC_CLK_ENABLE. It seems to make sense to check this value
> for macpower no matter what chip it is. I think I can make it more
> self-expressive
> as down below.
> 
>  if (val8 == 0xea || !(val16 & SYS_CLK_MAC_CLK_ENABLE))

Yes, please always use the descriptive defines rather than hard coding
the bit numbers.

> And per the comment, this code is for 92DU-VC S3 hang problem and I think an
> OR check for SYS_CLK_MAC_CLK_ENABLE is still safe for this.

Sounds reasonable - keep in mind that some of these bugs may have been
fixed for one chip, and then just copied forward.

Cheers,
Jes

Re: [PATCH] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU

[Jes Sorensen]

On 7/3/19 3:42 AM, Daniel Drake wrote:
> On Tue, Jul 2, 2019 at 8:42 PM Jes Sorensen  wrote:
>> We definitely don't want to bring over the vendor code, since it's a
>> pile of spaghetti, but we probably need to get something sorted. This
>> went down the drain when the bluetooth driver was added without taking
>> it into account - long after this driver was merged.
> 
> Yeah, I didn't mean bring over quite so literally.. Chris is studying
> it and figuring out the neatest way to reimplement the required bits.
> 
> As for the relationship with bluetooth.. actually the bug that Chris
> is working on here is that the rtl8xxxu wifi signal is totally
> unusable *until* the bluetooth driver is loaded.

So this is not my experience at all from when I wrote the code. The
8723bu dongle I used for it came up just fine.

> Once the bluetooth driver is loaded, at the point of bluetooth
> firmware upload, the rtl8xxxu signal magiaclly strength becomes good.
> I think this is consistent with other rtl8xxxu problem reports that we
> saw lying around, although they had not been diagnosed in so much
> detail.

See this is the very opposite of what I have experienced. The bluetooth
driver ruins the signal when it's loaded with my dongle.

> The rtl8723bu vendor driver does not suffer this problem, it works
> fine with or without the bluetooth driver in place.

My point is this seems to be very dongle dependent :( We have to be
careful not breaking it for some users while fixing it for others.

Cheers,
Jes

Re: [PATCH] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU

[Jes Sorensen]

On 6/27/19 5:52 AM, Chris Chiu wrote:
> The WiFi tx power of RTL8723BU is extremely low after booting. So
> the WiFi scan gives very limited AP list and it always fails to
> connect to the selected AP. This module only supports 1x1 antenna
> and the antenna is switched to bluetooth due to some incorrect
> register settings.
> 
> This commit hand over the antenna control to PTA, the wifi signal
> will be back to normal and the bluetooth scan can also work at the
> same time. However, the btcoexist still needs to be handled under
> different circumstances. If there's a BT connection established,
> the wifi still fails to connect until disconneting the BT.
> 
> Signed-off-by: Chris Chiu 
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 9 ++---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c  | 3 ++-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> index 3adb1d3d47ac..6c3c70d93ac1 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv 
> *priv)
>   /*
>* WLAN action by PTA
>*/
> - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04);
> + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c);
>  
>   /*
>* BT select S0/S1 controlled by WiFi
> @@ -1568,9 +1568,12 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv 
> *priv)
>   rtl8xxxu_gen2_h2c_cmd(priv, , sizeof(h2c.ant_sel_rsv));
>  
>   /*
> -  * 0x280, 0x00, 0x200, 0x80 - not clear
> +  * Different settings per different antenna position.
> +  * Antenna switch to BT: 0x280, 0x00 (inverse)
> +  * Antenna switch to WiFi: 0x0, 0x280 (inverse)
> +  * Antenna controlled by PTA: 0x200, 0x80 (inverse)
>*/
> - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00);
> + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80);
>  
>   /*
>* Software control, antenna at WiFi side
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 8136e268b4e6..87b2179a769e 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw 
> *hw)
>  
>   /* Check if MAC is already powered on */
>   val8 = rtl8xxxu_read8(priv, REG_CR);
> + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR);
>  
>   /*
>* 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 || !(val16 & BIT(11)))
>   macpower = false;
>   else
>   macpower = true;

This part I would like to ask you take a good look at the other chips to
make sure you don't break support for 8192cu, 8723au, 8188eu with this.

Cheers,
Jes

Re: [PATCH] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU

[Jes Sorensen]

On 7/1/19 4:27 AM, Daniel Drake wrote:
> Hi Chris,
> 
> On Thu, Jun 27, 2019 at 5:53 PM Chris Chiu  wrote:
>> The WiFi tx power of RTL8723BU is extremely low after booting. So
>> the WiFi scan gives very limited AP list and it always fails to
>> connect to the selected AP. This module only supports 1x1 antenna
>> and the antenna is switched to bluetooth due to some incorrect
>> register settings.
>>
>> This commit hand over the antenna control to PTA, the wifi signal
>> will be back to normal and the bluetooth scan can also work at the
>> same time. However, the btcoexist still needs to be handled under
>> different circumstances. If there's a BT connection established,
>> the wifi still fails to connect until disconneting the BT.
>>
>> Signed-off-by: Chris Chiu 
> 
> Really nice work finding this!
> 
> I know that after this change, you plan to bring over the btcoexist
> code from the vendor driver (or at least the minimum required code)
> for a more complete fix, but I'm curious how you found these magic
> register values and how they compare to the values used by the vendor
> driver with btcoexist?

We definitely don't want to bring over the vendor code, since it's a
pile of spaghetti, but we probably need to get something sorted. This
went down the drain when the bluetooth driver was added without taking
it into account - long after this driver was merged.

Cheers,
Jes

Re: [PATCH 80/87] net: hippi: remove memset after pci_alloc_consistent

[Jes Sorensen]

On 6/27/19 1:44 PM, Fuqian Huang wrote:
> pci_alloc_consistent calls dma_alloc_coherent directly.
> In commit af7ddd8a627c
> ("Merge tag 'dma-mapping-4.21' of 
> git://git.infradead.org/users/hch/dma-mapping"),
> dma_alloc_coherent has already zeroed the memory.
> So memset is not needed.
> 
> Signed-off-by: Fuqian Huang 
> ---
>  drivers/net/hippi/rrunner.c | 2 --
>  1 file changed, 2 deletions(-)

Acked-by: Jes Sorensen 

> diff --git a/drivers/net/hippi/rrunner.c b/drivers/net/hippi/rrunner.c
> index 7b9350dbebdd..2a6ec5394966 100644
> --- a/drivers/net/hippi/rrunner.c
> +++ b/drivers/net/hippi/rrunner.c
> @@ -1196,7 +1196,6 @@ static int rr_open(struct net_device *dev)
>   goto error;
>   }
>   rrpriv->rx_ctrl_dma = dma_addr;
> - memset(rrpriv->rx_ctrl, 0, 256*sizeof(struct ring_ctrl));
>  
>   rrpriv->info = pci_alloc_consistent(pdev, sizeof(struct rr_info),
>   _addr);
> @@ -1205,7 +1204,6 @@ static int rr_open(struct net_device *dev)
>   goto error;
>   }
>   rrpriv->info_dma = dma_addr;
> - memset(rrpriv->info, 0, sizeof(struct rr_info));
>   wmb();
>  
>   spin_lock_irqsave(>lock, flags);
> 

Re: [RFC PATCH v4] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver

[Jes Sorensen]

On 5/31/19 5:12 AM, Chris Chiu wrote:
> We have 3 laptops which connect the wifi by the same RTL8723BU.
> The PCI VID/PID of the wifi chip is 10EC:B720 which is supported.
> They have the same problem with the in-kernel rtl8xxxu driver, the
> iperf (as a client to an ethernet-connected server) gets ~1Mbps.
> Nevertheless, the signal strength is reported as around -40dBm,
> which is quite good. From the wireshark capture, the tx rate for each
> data and qos data packet is only 1Mbps. Compare to the Realtek driver
> at https://github.com/lwfinger/rtl8723bu, the same iperf test gets
> ~12Mbps or better. The signal strength is reported similarly around
> -40dBm. That's why we want to improve.
> 
> After reading the source code of the rtl8xxxu driver and Realtek's, the
> major difference is that Realtek's driver has a watchdog which will keep
> monitoring the signal quality and updating the rate mask just like the
> rtl8xxxu_gen2_update_rate_mask() does if signal quality changes.
> And this kind of watchdog also exists in rtlwifi driver of some specific
> chips, ex rtl8192ee, rtl8188ee, rtl8723ae, rtl8821ae...etc. They have
> the same member function named dm_watchdog and will invoke the
> corresponding dm_refresh_rate_adaptive_mask to adjust the tx rate
> mask.
> 
> With this commit, the tx rate of each data and qos data packet will
> be 39Mbps (MCS4) with the 0xF0 as the tx rate mask. The 20th bit
> to 23th bit means MCS4 to MCS7. It means that the firmware still picks
> the lowest rate from the rate mask and explains why the tx rate of
> data and qos data is always lowest 1Mbps because the default rate mask
> passed is always 0xFFF ranges from the basic CCK rate, OFDM rate,
> and MCS rate. However, with Realtek's driver, the tx rate observed from
> wireshark under the same condition is almost 65Mbps or 72Mbps.
> 
> I believe the firmware of RTL8723BU may need fix. And I think we
> can still bring in the dm_watchdog as rtlwifi to improve from the
> driver side. Please leave precious comments for my commits and
> suggest what I can do better. Or suggest if there's any better idea
> to fix this. Thanks.
> 
> Signed-off-by: Chris Chiu 

I am really pleased to see you're investigating some of these issues,
since I've been pretty swamped and not had time to work on this driver
for a long time.

The firmware should allow for two rate modes, either firmware handled or
controlled by the driver. Ideally we would want the driver to handle it,
but I never was able to make that work reliable.

This fix should at least improve the situation, and it may explain some
of the performance issues with the 8192eu as well?

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index 8828baf26e7b..216f603827a8 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -1195,6 +1195,44 @@ struct rtl8723bu_c2h {
>  
>  struct rtl8xxxu_fileops;
>  
> +/*mlme related.*/
> +enum wireless_mode {
> + WIRELESS_MODE_UNKNOWN = 0,
> + /* Sub-Element */
> + WIRELESS_MODE_B = BIT(0),
> + WIRELESS_MODE_G = BIT(1),
> + WIRELESS_MODE_A = BIT(2),
> + WIRELESS_MODE_N_24G = BIT(3),
> + WIRELESS_MODE_N_5G = BIT(4),
> + WIRELESS_AUTO = BIT(5),
> + WIRELESS_MODE_AC = BIT(6),
> + WIRELESS_MODE_MAX = 0x7F,
> +};
> +
> +/* from rtlwifi/wifi.h */
> +enum ratr_table_mode_new {
> + RATEID_IDX_BGN_40M_2SS = 0,
> + RATEID_IDX_BGN_40M_1SS = 1,
> + RATEID_IDX_BGN_20M_2SS_BN = 2,
> + RATEID_IDX_BGN_20M_1SS_BN = 3,
> + RATEID_IDX_GN_N2SS = 4,
> + RATEID_IDX_GN_N1SS = 5,
> + RATEID_IDX_BG = 6,
> + RATEID_IDX_G = 7,
> + RATEID_IDX_B = 8,
> + RATEID_IDX_VHT_2SS = 9,
> + RATEID_IDX_VHT_1SS = 10,
> + RATEID_IDX_MIX1 = 11,
> + RATEID_IDX_MIX2 = 12,
> + RATEID_IDX_VHT_3SS = 13,
> + RATEID_IDX_BGN_3SS = 14,
> +};
> +
> +#define RTL8XXXU_RATR_STA_INIT 0
> +#define RTL8XXXU_RATR_STA_HIGH 1
> +#define RTL8XXXU_RATR_STA_MID  2
> +#define RTL8XXXU_RATR_STA_LOW  3
> +

>  extern struct rtl8xxxu_fileops rtl8192cu_fops;
>  extern struct rtl8xxxu_fileops rtl8192eu_fops;
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c 
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> index 26b674aca125..2071ab9fd001 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
> @@ -1645,6 +1645,148 @@ static void rtl8723bu_init_statistics(struct 
> rtl8xxxu_priv *priv)
>   rtl8xxxu_write32(priv, REG_OFDM0_FA_RSTC, val32);
>  }
>  
> +static u8 rtl8723b_signal_to_rssi(int signal)
> +{
> + if (signal < -95)
> + signal = -95;
> + return (u8)(signal + 95);
> +}

Could you make this more generic so it can be used by the other sub-drivers?

> +static void rtl8723b_refresh_rate_mask(struct rtl8xxxu_priv *priv,
> +

Re: [PATCH] rtl8xxxu: Add rtl8188ctv support

[Jes Sorensen]

On 09/04/2018 04:16 AM, Kalle Valo wrote:
> Aleksei Mamlin  wrote:
> 
>> The Realtek rtl8188ctv (0x0bda:0x018a) is a highly integrated single-chip
>> WLAN USB2.0 network interface controller.
>>
>> Currently rtl8188ctv is supported by rtlwifi driver.
>> It is similar to the rtl8188cus(0x0bda:0x818a) and uses the same config.
>>
>> Signed-off-by: Aleksei Mamlin 
> 
> Patch applied to wireless-drivers-next.git, thanks.
> 
> 514502c3a70b rtl8xxxu: Add rtl8188ctv support

Looks good to me too - assume it's been tested?

Thanks,
Jes

Re: [PATCH] rtl8xxxu: Add rtl8188ctv support

[Jes Sorensen]

On 09/04/2018 04:16 AM, Kalle Valo wrote:
> Aleksei Mamlin  wrote:
> 
>> The Realtek rtl8188ctv (0x0bda:0x018a) is a highly integrated single-chip
>> WLAN USB2.0 network interface controller.
>>
>> Currently rtl8188ctv is supported by rtlwifi driver.
>> It is similar to the rtl8188cus(0x0bda:0x818a) and uses the same config.
>>
>> Signed-off-by: Aleksei Mamlin 
> 
> Patch applied to wireless-drivers-next.git, thanks.
> 
> 514502c3a70b rtl8xxxu: Add rtl8188ctv support

Looks good to me too - assume it's been tested?

Thanks,
Jes

Re: [PATCH 1/5] PCI/AER: Define and allocate aer_stats structure for AER capable devices

[Jes Sorensen]

On 05/23/2018 10:26 AM, Matthew Wilcox wrote:
> On Wed, May 23, 2018 at 10:20:10AM -0400, Jes Sorensen wrote:
>>> +++ b/drivers/pci/pcie/aer/aerdrv_stats.c
>>> @@ -0,0 +1,64 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>> Fix the formatting please - that gross // gibberish doesn't belong there.
> 
> Sorry, Jes.  The Chief Penguin has Spoken, and that's the preferred
> syntax:
> 
> 2. Style:
> 
>The SPDX license identifier is added in form of a comment.  The comment
>style depends on the file type::
> 
>   C source: // SPDX-License-Identifier: 
> 
> (you can dig up the discussion around this on the mailing list if you
> like.  Linus actually thinks that C++ single-line comments are one of
> the few things that language got right)

Well I'll agree to disagree with Linus on this one. It's ugly as fsck
and allows for ambiguous statements in the code.

Jes

Re: [PATCH 1/5] PCI/AER: Define and allocate aer_stats structure for AER capable devices

[Jes Sorensen]

On 05/23/2018 10:26 AM, Matthew Wilcox wrote:
> On Wed, May 23, 2018 at 10:20:10AM -0400, Jes Sorensen wrote:
>>> +++ b/drivers/pci/pcie/aer/aerdrv_stats.c
>>> @@ -0,0 +1,64 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>> Fix the formatting please - that gross // gibberish doesn't belong there.
> 
> Sorry, Jes.  The Chief Penguin has Spoken, and that's the preferred
> syntax:
> 
> 2. Style:
> 
>The SPDX license identifier is added in form of a comment.  The comment
>style depends on the file type::
> 
>   C source: // SPDX-License-Identifier: 
> 
> (you can dig up the discussion around this on the mailing list if you
> like.  Linus actually thinks that C++ single-line comments are one of
> the few things that language got right)

Well I'll agree to disagree with Linus on this one. It's ugly as fsck
and allows for ambiguous statements in the code.

Jes

Re: [PATCH 1/5] PCI/AER: Define and allocate aer_stats structure for AER capable devices

[Jes Sorensen]

On 05/23/2018 10:26 AM, Alex G. wrote:
> On 05/23/2018 09:20 AM, Jes Sorensen wrote:
>> On 05/22/2018 06:28 PM, Rajat Jain wrote:
>>> new file mode 100644
>>> index ..b9f251992209
>>> --- /dev/null
>>> +++ b/drivers/pci/pcie/aer/aerdrv_stats.c
>>> @@ -0,0 +1,64 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>> Fix the formatting please - that gross // gibberish doesn't belong there.
> 
> Deep breath in. Deep breath out.
> 
> git grep SPDX
> 
> Although I don't like it, this format is already too common.

So? Just because some people did something wrong doesn't mean you should
continue to do it.

Jes

Re: [PATCH 1/5] PCI/AER: Define and allocate aer_stats structure for AER capable devices

[Jes Sorensen]

On 05/23/2018 10:26 AM, Alex G. wrote:
> On 05/23/2018 09:20 AM, Jes Sorensen wrote:
>> On 05/22/2018 06:28 PM, Rajat Jain wrote:
>>> new file mode 100644
>>> index ..b9f251992209
>>> --- /dev/null
>>> +++ b/drivers/pci/pcie/aer/aerdrv_stats.c
>>> @@ -0,0 +1,64 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>
>> Fix the formatting please - that gross // gibberish doesn't belong there.
> 
> Deep breath in. Deep breath out.
> 
> git grep SPDX
> 
> Although I don't like it, this format is already too common.

So? Just because some people did something wrong doesn't mean you should
continue to do it.

Jes

Re: [PATCH 1/5] PCI/AER: Define and allocate aer_stats structure for AER capable devices

[Jes Sorensen]

On 05/22/2018 06:28 PM, Rajat Jain wrote:
> Define a structure to hold the AER statistics. There are 2 groups
> of statistics: dev_* counters that are to be collected for all AER
> capable devices and rootport_* counters that are collected for all
> (AER capable) rootports only. Allocate and free this structure when
> device is added or released (thus counters survive the lifetime of the
> device).
> 
> Add a new file aerdrv_stats.c to hold the AER stats collection logic.
> 
> Signed-off-by: Rajat Jain 
> ---
>  drivers/pci/pcie/aer/Makefile   |  2 +-
>  drivers/pci/pcie/aer/aerdrv.h   |  6 +++
>  drivers/pci/pcie/aer/aerdrv_core.c  |  9 
>  drivers/pci/pcie/aer/aerdrv_stats.c | 64 +
>  drivers/pci/probe.c |  1 +
>  include/linux/pci.h |  3 ++
>  6 files changed, 84 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/pcie/aer/aerdrv_stats.c
> 
> diff --git a/drivers/pci/pcie/aer/Makefile b/drivers/pci/pcie/aer/Makefile
>  
> -aerdriver-objs := aerdrv_errprint.o aerdrv_core.o aerdrv.o
> +aerdriver-objs := aerdrv_errprint.o aerdrv_core.o aerdrv.o aerdrv_stats.o
>  aerdriver-$(CONFIG_ACPI) += aerdrv_acpi.o
>  
>  obj-$(CONFIG_PCIEAER_INJECT) += aer_inject.o
> diff --git a/drivers/pci/pcie/aer/aerdrv.h b/drivers/pci/pcie/aer/aerdrv.h
> index b4c950683cc7..d8b9fba536ed 100644
> --- a/drivers/pci/pcie/aer/aerdrv.h
> +++ b/drivers/pci/pcie/aer/aerdrv.h
> @@ -33,6 +33,10 @@
>   PCI_ERR_UNC_MALF_TLP)
>  
>  #define AER_MAX_MULTI_ERR_DEVICES5   /* Not likely to have more */
> +
> +#define AER_MAX_TYPEOF_CORRECTABLE_ERRS 16   /* as per PCI_ERR_COR_STATUS */
> +#define AER_MAX_TYPEOF_UNCORRECTABLE_ERRS 26 /* as per PCI_ERR_UNCOR_STATUS*/
> +
>  struct aer_err_info {
>   struct pci_dev *dev[AER_MAX_MULTI_ERR_DEVICES];
>   int error_dev_num;
> @@ -81,6 +85,8 @@ void aer_isr(struct work_struct *work);
>  void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
>  void aer_print_port_info(struct pci_dev *dev, struct aer_err_info *info);
>  irqreturn_t aer_irq(int irq, void *context);
> +int pci_aer_stats_init(struct pci_dev *pdev);
> +void pci_aer_stats_exit(struct pci_dev *pdev);
>  
>  #ifdef CONFIG_ACPI_APEI
>  int pcie_aer_get_firmware_first(struct pci_dev *pci_dev);
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c 
> b/drivers/pci/pcie/aer/aerdrv_core.c
> index 36e622d35c48..42a6f913069a 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -95,9 +95,18 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>  int pci_aer_init(struct pci_dev *dev)
>  {
>   dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +
> + if (!dev->aer_cap || pci_aer_stats_init(dev))
> + return -EIO;
> +
>   return pci_cleanup_aer_error_status_regs(dev);
>  }
>  
> +void pci_aer_exit(struct pci_dev *dev)
> +{
> + pci_aer_stats_exit(dev);
> +}
> +
>  /**
>   * add_error_device - list device to be handled
>   * @e_info: pointer to error info
> diff --git a/drivers/pci/pcie/aer/aerdrv_stats.c 
> b/drivers/pci/pcie/aer/aerdrv_stats.c
> new file mode 100644
> index ..b9f251992209
> --- /dev/null
> +++ b/drivers/pci/pcie/aer/aerdrv_stats.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0

Fix the formatting please - that gross // gibberish doesn't belong there.

Jes

Re: [PATCH 1/5] PCI/AER: Define and allocate aer_stats structure for AER capable devices

[Jes Sorensen]

On 05/22/2018 06:28 PM, Rajat Jain wrote:
> Define a structure to hold the AER statistics. There are 2 groups
> of statistics: dev_* counters that are to be collected for all AER
> capable devices and rootport_* counters that are collected for all
> (AER capable) rootports only. Allocate and free this structure when
> device is added or released (thus counters survive the lifetime of the
> device).
> 
> Add a new file aerdrv_stats.c to hold the AER stats collection logic.
> 
> Signed-off-by: Rajat Jain 
> ---
>  drivers/pci/pcie/aer/Makefile   |  2 +-
>  drivers/pci/pcie/aer/aerdrv.h   |  6 +++
>  drivers/pci/pcie/aer/aerdrv_core.c  |  9 
>  drivers/pci/pcie/aer/aerdrv_stats.c | 64 +
>  drivers/pci/probe.c |  1 +
>  include/linux/pci.h |  3 ++
>  6 files changed, 84 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/pcie/aer/aerdrv_stats.c
> 
> diff --git a/drivers/pci/pcie/aer/Makefile b/drivers/pci/pcie/aer/Makefile
>  
> -aerdriver-objs := aerdrv_errprint.o aerdrv_core.o aerdrv.o
> +aerdriver-objs := aerdrv_errprint.o aerdrv_core.o aerdrv.o aerdrv_stats.o
>  aerdriver-$(CONFIG_ACPI) += aerdrv_acpi.o
>  
>  obj-$(CONFIG_PCIEAER_INJECT) += aer_inject.o
> diff --git a/drivers/pci/pcie/aer/aerdrv.h b/drivers/pci/pcie/aer/aerdrv.h
> index b4c950683cc7..d8b9fba536ed 100644
> --- a/drivers/pci/pcie/aer/aerdrv.h
> +++ b/drivers/pci/pcie/aer/aerdrv.h
> @@ -33,6 +33,10 @@
>   PCI_ERR_UNC_MALF_TLP)
>  
>  #define AER_MAX_MULTI_ERR_DEVICES5   /* Not likely to have more */
> +
> +#define AER_MAX_TYPEOF_CORRECTABLE_ERRS 16   /* as per PCI_ERR_COR_STATUS */
> +#define AER_MAX_TYPEOF_UNCORRECTABLE_ERRS 26 /* as per PCI_ERR_UNCOR_STATUS*/
> +
>  struct aer_err_info {
>   struct pci_dev *dev[AER_MAX_MULTI_ERR_DEVICES];
>   int error_dev_num;
> @@ -81,6 +85,8 @@ void aer_isr(struct work_struct *work);
>  void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
>  void aer_print_port_info(struct pci_dev *dev, struct aer_err_info *info);
>  irqreturn_t aer_irq(int irq, void *context);
> +int pci_aer_stats_init(struct pci_dev *pdev);
> +void pci_aer_stats_exit(struct pci_dev *pdev);
>  
>  #ifdef CONFIG_ACPI_APEI
>  int pcie_aer_get_firmware_first(struct pci_dev *pci_dev);
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c 
> b/drivers/pci/pcie/aer/aerdrv_core.c
> index 36e622d35c48..42a6f913069a 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -95,9 +95,18 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>  int pci_aer_init(struct pci_dev *dev)
>  {
>   dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +
> + if (!dev->aer_cap || pci_aer_stats_init(dev))
> + return -EIO;
> +
>   return pci_cleanup_aer_error_status_regs(dev);
>  }
>  
> +void pci_aer_exit(struct pci_dev *dev)
> +{
> + pci_aer_stats_exit(dev);
> +}
> +
>  /**
>   * add_error_device - list device to be handled
>   * @e_info: pointer to error info
> diff --git a/drivers/pci/pcie/aer/aerdrv_stats.c 
> b/drivers/pci/pcie/aer/aerdrv_stats.c
> new file mode 100644
> index ..b9f251992209
> --- /dev/null
> +++ b/drivers/pci/pcie/aer/aerdrv_stats.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0

Fix the formatting please - that gross // gibberish doesn't belong there.

Jes

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] 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: [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 
> ---
> 
> 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 


> 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: [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: net...@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] 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 
Cc: linux-hi...@sunsite.dk
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook 
---
  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/11/2017 04:41 AM, Kalle Valo wrote:

Jes Sorensen  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: net...@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] 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 
Cc: Kalle Valo 
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva 
---
  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 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 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 

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 10:19 AM, Arnd Bergmann wrote:

On Tue, May 16, 2017 at 3:44 PM, Jes Sorensen  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] 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] format-security: move static strings to const

[Jes Sorensen]

On 04/05/2017 05:47 PM, Kees Cook wrote:

While examining output from trial builds with -Wformat-security enabled,
many strings were found that should be defined as "const", or as a char
array instead of char pointer. This makes some static analysis easier,
by producing fewer false positives.

As these are all trivial changes, it seemed best to put them all in
a single patch rather than chopping them up per maintainer.

Signed-off-by: Kees Cook 
---
 arch/arm/mach-omap2/board-n8x0.c  |  2 +-
 arch/mips/dec/prom/init.c |  6 +++---
 arch/mips/kernel/traps.c  |  4 ++--
 drivers/char/dsp56k.c |  2 +-
 drivers/cpufreq/powernow-k8.c |  3 ++-
 drivers/gpu/drm/drm_fb_helper.c   |  2 +-
 drivers/net/ethernet/amd/atarilance.c |  4 ++--
 drivers/net/ethernet/amd/declance.c   |  2 +-
 drivers/net/ethernet/amd/sun3lance.c  |  3 ++-
 drivers/net/ethernet/cirrus/mac89x0.c |  2 +-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h |  2 +-
 drivers/net/ethernet/natsemi/sonic.h  |  2 +-
 drivers/net/ethernet/toshiba/tc35815.c|  2 +-
 drivers/net/fddi/defxx.c  |  2 +-
 drivers/net/hippi/rrunner.c   |  3 ++-
 drivers/staging/most/mostcore/core.c  |  2 +-
 drivers/tty/n_hdlc.c  | 10 +-
 drivers/tty/serial/st-asc.c   |  2 +-
 net/decnet/af_decnet.c|  3 ++-
 19 files changed, 31 insertions(+), 27 deletions(-)


rrunner.c changes look fine to me.

Jes

Re: [PATCH] format-security: move static strings to const

[Jes Sorensen]

On 04/05/2017 05:47 PM, Kees Cook wrote:

While examining output from trial builds with -Wformat-security enabled,
many strings were found that should be defined as "const", or as a char
array instead of char pointer. This makes some static analysis easier,
by producing fewer false positives.

As these are all trivial changes, it seemed best to put them all in
a single patch rather than chopping them up per maintainer.

Signed-off-by: Kees Cook 
---
 arch/arm/mach-omap2/board-n8x0.c  |  2 +-
 arch/mips/dec/prom/init.c |  6 +++---
 arch/mips/kernel/traps.c  |  4 ++--
 drivers/char/dsp56k.c |  2 +-
 drivers/cpufreq/powernow-k8.c |  3 ++-
 drivers/gpu/drm/drm_fb_helper.c   |  2 +-
 drivers/net/ethernet/amd/atarilance.c |  4 ++--
 drivers/net/ethernet/amd/declance.c   |  2 +-
 drivers/net/ethernet/amd/sun3lance.c  |  3 ++-
 drivers/net/ethernet/cirrus/mac89x0.c |  2 +-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h |  2 +-
 drivers/net/ethernet/natsemi/sonic.h  |  2 +-
 drivers/net/ethernet/toshiba/tc35815.c|  2 +-
 drivers/net/fddi/defxx.c  |  2 +-
 drivers/net/hippi/rrunner.c   |  3 ++-
 drivers/staging/most/mostcore/core.c  |  2 +-
 drivers/tty/n_hdlc.c  | 10 +-
 drivers/tty/serial/st-asc.c   |  2 +-
 net/decnet/af_decnet.c|  3 ++-
 19 files changed, 31 insertions(+), 27 deletions(-)


rrunner.c changes look fine to me.

Jes

Re: [PATCHv2] mdadm.c: fix compile error "switch condition has boolean value"

[Jes Sorensen]

On 03/30/2017 12:58 PM, Gioh Kim wrote:

Remove a boolean expression in switch condition
to prevent compile error of some compilers,
for example, gcc version 5.2.1 20151010 (Ubuntu 5.2.1-22ubuntu2).

Signed-off-by: Gioh Kim 
---
 mdadm.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)


Applied!

Thanks,
Jes


diff --git a/mdadm.c b/mdadm.c
index 0f32773..d6b5437 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1965,14 +1965,12 @@ static int misc_list(struct mddev_dev *devlist,
rv |= SetAction(dv->devname, c->action);
continue;
}
-   switch(dv->devname[0] == '/') {
-   case 0:
-   mdfd = open_dev(dv->devname);
-   if (mdfd >= 0)
-   break;
-   case 1:
-   mdfd = open_mddev(dv->devname, 1);
-   }
+
+   if (dv->devname[0] != '/')
+   mdfd = open_dev(dv->devname);
+   if (dv->devname[0] == '/' || mdfd < 0)
+   mdfd = open_mddev(dv->devname, 1);
+
if (mdfd >= 0) {
switch(dv->disposition) {
case 'R':

Re: [PATCHv2] mdadm.c: fix compile error "switch condition has boolean value"

[Jes Sorensen]

On 03/30/2017 12:58 PM, Gioh Kim wrote:

Remove a boolean expression in switch condition
to prevent compile error of some compilers,
for example, gcc version 5.2.1 20151010 (Ubuntu 5.2.1-22ubuntu2).

Signed-off-by: Gioh Kim 
---
 mdadm.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)


Applied!

Thanks,
Jes


diff --git a/mdadm.c b/mdadm.c
index 0f32773..d6b5437 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1965,14 +1965,12 @@ static int misc_list(struct mddev_dev *devlist,
rv |= SetAction(dv->devname, c->action);
continue;
}
-   switch(dv->devname[0] == '/') {
-   case 0:
-   mdfd = open_dev(dv->devname);
-   if (mdfd >= 0)
-   break;
-   case 1:
-   mdfd = open_mddev(dv->devname, 1);
-   }
+
+   if (dv->devname[0] != '/')
+   mdfd = open_dev(dv->devname);
+   if (dv->devname[0] == '/' || mdfd < 0)
+   mdfd = open_mddev(dv->devname, 1);
+
if (mdfd >= 0) {
switch(dv->disposition) {
case 'R':

Re: [PATCH 2/2] mdadm.c: fix compile error "switch condition has boolean value"

[Jes Sorensen]

On 03/30/2017 03:52 AM, Gioh Kim wrote:

On Thu, Mar 30, 2017 at 08:38:35AM +1100, NeilBrown wrote:

On Thu, Mar 30 2017, jes.soren...@gmail.com wrote:


Gioh Kim  writes:

Remove a boolean expression in switch condition
to prevent compile error of some compilers.


Please be specific, which compile is unable to handle this?


Signed-off-by: Gioh Kim 
---
 mdadm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mdadm.c b/mdadm.c
index 08ddcab..a98a051 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1905,11 +1905,11 @@ static int misc_list(struct mddev_dev *devlist,
rv |= SetAction(dv->devname, c->action);
continue;
}
-   switch(dv->devname[0] == '/') {
-   case 0:
+   switch(dv->devname[0]) {
+   default:
mdfd = open_dev(dv->devname);
if (mdfd >= 0) break;
-   case 1:
+   case '/':
mdfd = open_mddev(dv->devname, 1);
}
if (mdfd>=0) {


While I agree the original code is ugly, I am not convinced your
replacement is a lot prettier.



Maybe

if (dv->devname[0] == '/' ||
(mdfd = open_dev(dv->devname)) < 0)
mdfd = open_mddev(dv->devname, 1);

??
NeilBrown


Yes, the initial version I thought was:

if (dev->devname[0] != '/')
mdfd = open_dev(dv->devname);
if (dev->devname[0] == '/' || mdfd < 0)
mdfd = open_mddev(dv->devname, 1);

But I thought you have a reason to use switch-case expression,
so I kept switch-case.
If you agree, I'll send v2 patch using if-expression.



I like this solution better, I much favor code that is more explicit in 
what it does. Request less brain capacity for me to read :)


Cheers,
Jes

Re: [PATCH 2/2] mdadm.c: fix compile error "switch condition has boolean value"

[Jes Sorensen]

On 03/30/2017 03:52 AM, Gioh Kim wrote:

On Thu, Mar 30, 2017 at 08:38:35AM +1100, NeilBrown wrote:

On Thu, Mar 30 2017, jes.soren...@gmail.com wrote:


Gioh Kim  writes:

Remove a boolean expression in switch condition
to prevent compile error of some compilers.


Please be specific, which compile is unable to handle this?


Signed-off-by: Gioh Kim 
---
 mdadm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mdadm.c b/mdadm.c
index 08ddcab..a98a051 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1905,11 +1905,11 @@ static int misc_list(struct mddev_dev *devlist,
rv |= SetAction(dv->devname, c->action);
continue;
}
-   switch(dv->devname[0] == '/') {
-   case 0:
+   switch(dv->devname[0]) {
+   default:
mdfd = open_dev(dv->devname);
if (mdfd >= 0) break;
-   case 1:
+   case '/':
mdfd = open_mddev(dv->devname, 1);
}
if (mdfd>=0) {


While I agree the original code is ugly, I am not convinced your
replacement is a lot prettier.



Maybe

if (dv->devname[0] == '/' ||
(mdfd = open_dev(dv->devname)) < 0)
mdfd = open_mddev(dv->devname, 1);

??
NeilBrown


Yes, the initial version I thought was:

if (dev->devname[0] != '/')
mdfd = open_dev(dv->devname);
if (dev->devname[0] == '/' || mdfd < 0)
mdfd = open_mddev(dv->devname, 1);

But I thought you have a reason to use switch-case expression,
so I kept switch-case.
If you agree, I'll send v2 patch using if-expression.



I like this solution better, I much favor code that is more explicit in 
what it does. Request less brain capacity for me to read :)


Cheers,
Jes

Re: [PATCH 2/2] mdadm.c: fix compile error "switch condition has boolean value"

[jes . sorensen]

Gioh Kim  writes:
> Remove a boolean expression in switch condition
> to prevent compile error of some compilers.

Please be specific, which compile is unable to handle this?

> Signed-off-by: Gioh Kim 
> ---
>  mdadm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/mdadm.c b/mdadm.c
> index 08ddcab..a98a051 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1905,11 +1905,11 @@ static int misc_list(struct mddev_dev *devlist,
>   rv |= SetAction(dv->devname, c->action);
>   continue;
>   }
> - switch(dv->devname[0] == '/') {
> - case 0:
> + switch(dv->devname[0]) {
> + default:
>   mdfd = open_dev(dv->devname);
>   if (mdfd >= 0) break;
> - case 1:
> + case '/':
>   mdfd = open_mddev(dv->devname, 1);  
>   }
>   if (mdfd>=0) {

While I agree the original code is ugly, I am not convinced your
replacement is a lot prettier.

Thanks,
Jes

Re: [PATCH 2/2] mdadm.c: fix compile error "switch condition has boolean value"

[jes . sorensen]

Gioh Kim  writes:
> Remove a boolean expression in switch condition
> to prevent compile error of some compilers.

Please be specific, which compile is unable to handle this?

> Signed-off-by: Gioh Kim 
> ---
>  mdadm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/mdadm.c b/mdadm.c
> index 08ddcab..a98a051 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1905,11 +1905,11 @@ static int misc_list(struct mddev_dev *devlist,
>   rv |= SetAction(dv->devname, c->action);
>   continue;
>   }
> - switch(dv->devname[0] == '/') {
> - case 0:
> + switch(dv->devname[0]) {
> + default:
>   mdfd = open_dev(dv->devname);
>   if (mdfd >= 0) break;
> - case 1:
> + case '/':
>   mdfd = open_mddev(dv->devname, 1);  
>   }
>   if (mdfd>=0) {

While I agree the original code is ugly, I am not convinced your
replacement is a lot prettier.

Thanks,
Jes

Re: [PATCH 1/2] super1: replace hard-coded values with bit definitions

[jes . sorensen]

Gioh Kim  writes:
> Some hard-coded values for disk status are replaced
> with bit definitions.
>
> Signed-off-by: Gioh Kim 
> ---
>  super1.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Applied!

Please use --cover-letter when you send out multi-commit patch sets,
which includes a short description of what the set includes. That way I
can just respond to the cover letter if all the patches are applied.

Thanks,
Jes

Re: [PATCH 1/2] super1: replace hard-coded values with bit definitions

[jes . sorensen]

Gioh Kim  writes:
> Some hard-coded values for disk status are replaced
> with bit definitions.
>
> Signed-off-by: Gioh Kim 
> ---
>  super1.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Applied!

Please use --cover-letter when you send out multi-commit patch sets,
which includes a short description of what the set includes. That way I
can just respond to the cover letter if all the patches are applied.

Thanks,
Jes

Re: [PATCHv2 1/2] super1: ignore failfast flag for setting device role

[jes . sorensen]

Gioh Kim  writes:
> There is corner case for setting device role,
> if new device has failfast flag.
> The failfast flag should be ignored.
>
> Signed-off-by: Gioh Kim 
> Signed-off-by: Jack Wang 
> ---
>  super1.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Applied!

Ideally I would like to get rid of the hardcoded values and use the bit
defines instead, but the original code did the same, so I am going to
take it.

I am leaving out the second patch for now, per Neil's comments. If you
feel strongly about it, please conveince Neil first :)

Thanks,
Jes

Re: [PATCHv2 1/2] super1: ignore failfast flag for setting device role

[jes . sorensen]

Gioh Kim  writes:
> There is corner case for setting device role,
> if new device has failfast flag.
> The failfast flag should be ignored.
>
> Signed-off-by: Gioh Kim 
> Signed-off-by: Jack Wang 
> ---
>  super1.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Applied!

Ideally I would like to get rid of the hardcoded values and use the bit
defines instead, but the original code did the same, so I am going to
take it.

I am leaving out the second patch for now, per Neil's comments. If you
feel strongly about it, please conveince Neil first :)

Thanks,
Jes

[PATCH 1/5] notify: Call mutex_destroy() before freeing mutex memory

[Jes Sorensen]

The conversion of notify from using a spinlock to a mutex missed
adding the call mutex_destroy().

Fixes: 986ab09 ("fsnotify: use a mutex instead of a spinlock to protect a 
groups mark list")
Signed-off-by: Jes Sorensen <jsoren...@fb.com>
Reviewed-by: Josef Bacik <jba...@fb.com>
---
 fs/notify/group.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/notify/group.c b/fs/notify/group.c
index fbe3cbe..d231be3 100644
--- a/fs/notify/group.c
+++ b/fs/notify/group.c
@@ -36,6 +36,7 @@ static void fsnotify_final_destroy_group(struct 
fsnotify_group *group)
if (group->ops->free_group_priv)
group->ops->free_group_priv(group);
 
+   mutex_destroy(>mark_mutex);
kfree(group);
 }
 
-- 
2.9.3

[PATCH 1/5] notify: Call mutex_destroy() before freeing mutex memory

[Jes Sorensen]

The conversion of notify from using a spinlock to a mutex missed
adding the call mutex_destroy().

Fixes: 986ab09 ("fsnotify: use a mutex instead of a spinlock to protect a 
groups mark list")
Signed-off-by: Jes Sorensen 
Reviewed-by: Josef Bacik 
---
 fs/notify/group.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/notify/group.c b/fs/notify/group.c
index fbe3cbe..d231be3 100644
--- a/fs/notify/group.c
+++ b/fs/notify/group.c
@@ -36,6 +36,7 @@ static void fsnotify_final_destroy_group(struct 
fsnotify_group *group)
if (group->ops->free_group_priv)
group->ops->free_group_priv(group);
 
+   mutex_destroy(>mark_mutex);
kfree(group);
 }
 
-- 
2.9.3

[PATCH 5/5] inotify: Avoid taking spinlock for each event processed in read()

[Jes Sorensen]

Splice off the notification list while processing read events, which allows
it to be processed without taking and releasing the spinlock for each
event.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
Reviewed-by: Josef Bacik <jba...@fb.com>
---
 fs/notify/inotify/inotify_user.c | 28 
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 4a93750..45b1f69 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -224,22 +224,27 @@ static ssize_t inotify_read(struct file *file, char 
__user *buf,
struct fsnotify_group *group;
struct fsnotify_event *kevent;
char __user *start;
-   int ret;
+   int ret, consumed = 0;
DEFINE_WAIT_FUNC(wait, woken_wake_function);
+   LIST_HEAD(tmp_list);
 
start = buf;
group = file->private_data;
 
mutex_lock(>inotify_data.consumer_mutex);
add_wait_queue(>notification_waitq, );
+
+   spin_lock(>notification_lock);
+   list_splice_init(>notification_list, _list);
+   spin_unlock(>notification_lock);
+
while (1) {
-   spin_lock(>notification_lock);
-   kevent = get_one_event(>notification_list, count);
-   spin_unlock(>notification_lock);
+   kevent = get_one_event(_list, count);
 
pr_debug("%s: group=%p kevent=%p\n", __func__, group, kevent);
 
if (kevent) {
+   consumed++;
ret = PTR_ERR(kevent);
if (IS_ERR(kevent))
break;
@@ -262,8 +267,23 @@ static ssize_t inotify_read(struct file *file, char __user 
*buf,
if (start != buf)
break;
 
+   spin_lock(>notification_lock);
+   list_splice_init(_list, >notification_list);
+   group->q_len -= consumed;
+   consumed = 0;
+   spin_unlock(>notification_lock);
+
wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
+
+   spin_lock(>notification_lock);
+   list_splice_init(>notification_list, _list);
+   spin_unlock(>notification_lock);
}
+   spin_lock(>notification_lock);
+   list_splice(_list, >notification_list);
+   group->q_len -= consumed;
+   spin_unlock(>notification_lock);
+
remove_wait_queue(>notification_waitq, );
mutex_unlock(>inotify_data.consumer_mutex);
 
-- 
2.9.3

[PATCH 5/5] inotify: Avoid taking spinlock for each event processed in read()

[Jes Sorensen]

Splice off the notification list while processing read events, which allows
it to be processed without taking and releasing the spinlock for each
event.

Signed-off-by: Jes Sorensen 
Reviewed-by: Josef Bacik 
---
 fs/notify/inotify/inotify_user.c | 28 
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 4a93750..45b1f69 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -224,22 +224,27 @@ static ssize_t inotify_read(struct file *file, char 
__user *buf,
struct fsnotify_group *group;
struct fsnotify_event *kevent;
char __user *start;
-   int ret;
+   int ret, consumed = 0;
DEFINE_WAIT_FUNC(wait, woken_wake_function);
+   LIST_HEAD(tmp_list);
 
start = buf;
group = file->private_data;
 
mutex_lock(>inotify_data.consumer_mutex);
add_wait_queue(>notification_waitq, );
+
+   spin_lock(>notification_lock);
+   list_splice_init(>notification_list, _list);
+   spin_unlock(>notification_lock);
+
while (1) {
-   spin_lock(>notification_lock);
-   kevent = get_one_event(>notification_list, count);
-   spin_unlock(>notification_lock);
+   kevent = get_one_event(_list, count);
 
pr_debug("%s: group=%p kevent=%p\n", __func__, group, kevent);
 
if (kevent) {
+   consumed++;
ret = PTR_ERR(kevent);
if (IS_ERR(kevent))
break;
@@ -262,8 +267,23 @@ static ssize_t inotify_read(struct file *file, char __user 
*buf,
if (start != buf)
break;
 
+   spin_lock(>notification_lock);
+   list_splice_init(_list, >notification_list);
+   group->q_len -= consumed;
+   consumed = 0;
+   spin_unlock(>notification_lock);
+
wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
+
+   spin_lock(>notification_lock);
+   list_splice_init(>notification_list, _list);
+   spin_unlock(>notification_lock);
}
+   spin_lock(>notification_lock);
+   list_splice(_list, >notification_list);
+   group->q_len -= consumed;
+   spin_unlock(>notification_lock);
+
remove_wait_queue(>notification_waitq, );
mutex_unlock(>inotify_data.consumer_mutex);
 
-- 
2.9.3

[PATCH 4/5] inotify: switch get_one_event() to use fsnotify_list_*() helpers

[Jes Sorensen]

Preparing to switch inotify to code not taking the spinlock for each
event in read()

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
Reviewed-by: Josef Bacik <jba...@fb.com>
---
 fs/notify/inotify/inotify_user.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 6f5b360..4a93750 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -138,18 +138,18 @@ static int round_event_name_len(struct fsnotify_event 
*fsn_event)
  *
  * Called with the group->notification_lock held.
  */
-static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
+static struct fsnotify_event *get_one_event(struct list_head 
*notification_list,
size_t count)
 {
size_t event_size = sizeof(struct inotify_event);
struct fsnotify_event *event;
 
-   if (fsnotify_notify_queue_is_empty(group))
+   if (list_empty(notification_list))
return NULL;
 
-   event = fsnotify_peek_first_event(group);
+   event = fsnotify_list_peek_first_event(notification_list);
 
-   pr_debug("%s: group=%p event=%p\n", __func__, group, event);
+   pr_debug("%s: event=%p\n", __func__, event);
 
event_size += round_event_name_len(event);
if (event_size > count)
@@ -157,7 +157,7 @@ static struct fsnotify_event *get_one_event(struct 
fsnotify_group *group,
 
/* held the notification_lock the whole time, so this is the
 * same event we peeked above */
-   fsnotify_remove_first_event(group);
+   fsnotify_list_remove_first_event(notification_list);
 
return event;
 }
@@ -234,7 +234,7 @@ static ssize_t inotify_read(struct file *file, char __user 
*buf,
add_wait_queue(>notification_waitq, );
while (1) {
spin_lock(>notification_lock);
-   kevent = get_one_event(group, count);
+   kevent = get_one_event(>notification_list, count);
spin_unlock(>notification_lock);
 
pr_debug("%s: group=%p kevent=%p\n", __func__, group, kevent);
-- 
2.9.3

[PATCH 4/5] inotify: switch get_one_event() to use fsnotify_list_*() helpers

[Jes Sorensen]

Preparing to switch inotify to code not taking the spinlock for each
event in read()

Signed-off-by: Jes Sorensen 
Reviewed-by: Josef Bacik 
---
 fs/notify/inotify/inotify_user.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 6f5b360..4a93750 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -138,18 +138,18 @@ static int round_event_name_len(struct fsnotify_event 
*fsn_event)
  *
  * Called with the group->notification_lock held.
  */
-static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
+static struct fsnotify_event *get_one_event(struct list_head 
*notification_list,
size_t count)
 {
size_t event_size = sizeof(struct inotify_event);
struct fsnotify_event *event;
 
-   if (fsnotify_notify_queue_is_empty(group))
+   if (list_empty(notification_list))
return NULL;
 
-   event = fsnotify_peek_first_event(group);
+   event = fsnotify_list_peek_first_event(notification_list);
 
-   pr_debug("%s: group=%p event=%p\n", __func__, group, event);
+   pr_debug("%s: event=%p\n", __func__, event);
 
event_size += round_event_name_len(event);
if (event_size > count)
@@ -157,7 +157,7 @@ static struct fsnotify_event *get_one_event(struct 
fsnotify_group *group,
 
/* held the notification_lock the whole time, so this is the
 * same event we peeked above */
-   fsnotify_remove_first_event(group);
+   fsnotify_list_remove_first_event(notification_list);
 
return event;
 }
@@ -234,7 +234,7 @@ static ssize_t inotify_read(struct file *file, char __user 
*buf,
add_wait_queue(>notification_waitq, );
while (1) {
spin_lock(>notification_lock);
-   kevent = get_one_event(group, count);
+   kevent = get_one_event(>notification_list, count);
spin_unlock(>notification_lock);
 
pr_debug("%s: group=%p kevent=%p\n", __func__, group, kevent);
-- 
2.9.3

[PATCH 3/5] notify: Split up some fsnotify functions

[Jes Sorensen]

This splits up fsnotify_remove_first_event() and fsnotify_peek_first_event()
into a helper function with a wrapper, and introduces two new versions that
takes a list instead of the group as argument.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
Reviewed-by: Josef Bacik <jba...@fb.com>
---
 fs/notify/notification.c | 38 +++---
 include/linux/fsnotify_backend.h |  4 
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/fs/notify/notification.c b/fs/notify/notification.c
index 66f85c6..7b38cf8 100644
--- a/fs/notify/notification.c
+++ b/fs/notify/notification.c
@@ -144,26 +144,43 @@ int fsnotify_add_event(struct fsnotify_group *group,
  * Remove and return the first event from the notification list.  It is the
  * responsibility of the caller to destroy the obtained event
  */
-struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group 
*group)
+struct fsnotify_event *
+__fsnotify_remove_first_event(struct list_head *notification_list)
 {
struct fsnotify_event *event;
 
-   assert_spin_locked(>notification_lock);
-
-   pr_debug("%s: group=%p\n", __func__, group);
-
-   event = list_first_entry(>notification_list,
+   event = list_first_entry(notification_list,
 struct fsnotify_event, list);
/*
 * We need to init list head for the case of overflow event so that
 * check in fsnotify_add_event() works
 */
list_del_init(>list);
-   group->q_len--;
 
return event;
 }
 
+struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group 
*group)
+{
+   struct list_head *notification_list = >notification_list;
+
+   assert_spin_locked(>notification_lock);
+
+   pr_debug("%s: group=%p\n", __func__, group);
+
+   group->q_len--;
+   return __fsnotify_remove_first_event(notification_list);
+}
+
+/*
+ * Note this version doesn't update the queue depth counter.
+ */
+struct fsnotify_event *
+fsnotify_list_remove_first_event(struct list_head *notification_list)
+{
+   return __fsnotify_remove_first_event(notification_list);
+}
+
 /*
  * This will not remove the event, that must be done with
  * fsnotify_remove_first_event()
@@ -176,6 +193,13 @@ struct fsnotify_event *fsnotify_peek_first_event(struct 
fsnotify_group *group)
struct fsnotify_event, list);
 }
 
+struct fsnotify_event *
+fsnotify_list_peek_first_event(struct list_head *notification_list)
+{
+   return list_first_entry(notification_list,
+   struct fsnotify_event, list);
+}
+
 /*
  * Called when a group is being torn down to clean up any outstanding
  * event notifications.
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index e0686ed..80d4c3b 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -313,8 +313,12 @@ extern int fsnotify_add_event(struct fsnotify_group *group,
 extern bool fsnotify_notify_queue_is_empty(struct fsnotify_group *group);
 /* return, but do not dequeue the first event on the notification queue */
 extern struct fsnotify_event *fsnotify_peek_first_event(struct fsnotify_group 
*group);
+/* return, but do not dequeue the first event on the notification list */
+extern struct fsnotify_event *fsnotify_list_peek_first_event(struct list_head 
*notification_list);
 /* return AND dequeue the first event on the notification queue */
 extern struct fsnotify_event *fsnotify_remove_first_event(struct 
fsnotify_group *group);
+/* return AND dequeue the first event on the notification list */
+extern struct fsnotify_event *fsnotify_list_remove_first_event(struct 
list_head *notification_list);
 
 /* functions used to manipulate the marks attached to inodes */
 
-- 
2.9.3

[PATCH 3/5] notify: Split up some fsnotify functions

[Jes Sorensen]

This splits up fsnotify_remove_first_event() and fsnotify_peek_first_event()
into a helper function with a wrapper, and introduces two new versions that
takes a list instead of the group as argument.

Signed-off-by: Jes Sorensen 
Reviewed-by: Josef Bacik 
---
 fs/notify/notification.c | 38 +++---
 include/linux/fsnotify_backend.h |  4 
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/fs/notify/notification.c b/fs/notify/notification.c
index 66f85c6..7b38cf8 100644
--- a/fs/notify/notification.c
+++ b/fs/notify/notification.c
@@ -144,26 +144,43 @@ int fsnotify_add_event(struct fsnotify_group *group,
  * Remove and return the first event from the notification list.  It is the
  * responsibility of the caller to destroy the obtained event
  */
-struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group 
*group)
+struct fsnotify_event *
+__fsnotify_remove_first_event(struct list_head *notification_list)
 {
struct fsnotify_event *event;
 
-   assert_spin_locked(>notification_lock);
-
-   pr_debug("%s: group=%p\n", __func__, group);
-
-   event = list_first_entry(>notification_list,
+   event = list_first_entry(notification_list,
 struct fsnotify_event, list);
/*
 * We need to init list head for the case of overflow event so that
 * check in fsnotify_add_event() works
 */
list_del_init(>list);
-   group->q_len--;
 
return event;
 }
 
+struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group 
*group)
+{
+   struct list_head *notification_list = >notification_list;
+
+   assert_spin_locked(>notification_lock);
+
+   pr_debug("%s: group=%p\n", __func__, group);
+
+   group->q_len--;
+   return __fsnotify_remove_first_event(notification_list);
+}
+
+/*
+ * Note this version doesn't update the queue depth counter.
+ */
+struct fsnotify_event *
+fsnotify_list_remove_first_event(struct list_head *notification_list)
+{
+   return __fsnotify_remove_first_event(notification_list);
+}
+
 /*
  * This will not remove the event, that must be done with
  * fsnotify_remove_first_event()
@@ -176,6 +193,13 @@ struct fsnotify_event *fsnotify_peek_first_event(struct 
fsnotify_group *group)
struct fsnotify_event, list);
 }
 
+struct fsnotify_event *
+fsnotify_list_peek_first_event(struct list_head *notification_list)
+{
+   return list_first_entry(notification_list,
+   struct fsnotify_event, list);
+}
+
 /*
  * Called when a group is being torn down to clean up any outstanding
  * event notifications.
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index e0686ed..80d4c3b 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -313,8 +313,12 @@ extern int fsnotify_add_event(struct fsnotify_group *group,
 extern bool fsnotify_notify_queue_is_empty(struct fsnotify_group *group);
 /* return, but do not dequeue the first event on the notification queue */
 extern struct fsnotify_event *fsnotify_peek_first_event(struct fsnotify_group 
*group);
+/* return, but do not dequeue the first event on the notification list */
+extern struct fsnotify_event *fsnotify_list_peek_first_event(struct list_head 
*notification_list);
 /* return AND dequeue the first event on the notification queue */
 extern struct fsnotify_event *fsnotify_remove_first_event(struct 
fsnotify_group *group);
+/* return AND dequeue the first event on the notification list */
+extern struct fsnotify_event *fsnotify_list_remove_first_event(struct 
list_head *notification_list);
 
 /* functions used to manipulate the marks attached to inodes */
 
-- 
2.9.3

[PATCH 2/5] inotify: Use mutex to prevent threaded clients reading events out of order

[Jes Sorensen]

Introduces mutex in the read() path to prevent a threaded client reading
from the same fd consuming events out of order.

This will matter when avoiding taking the spinlock when consuming each
event in the read() path.

Signed-off-by: Jes Sorensen <jsoren...@fb.com>
Reviewed-by: Josef Bacik <jba...@fb.com>
---
 fs/notify/inotify/inotify_fsnotify.c | 1 +
 fs/notify/inotify/inotify_user.c | 4 
 include/linux/fsnotify_backend.h | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/fs/notify/inotify/inotify_fsnotify.c 
b/fs/notify/inotify/inotify_fsnotify.c
index 1aeb837..63c071f 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -168,6 +168,7 @@ static void inotify_free_group_priv(struct fsnotify_group 
*group)
idr_destroy(>inotify_data.idr);
if (group->inotify_data.ucounts)
dec_inotify_instances(group->inotify_data.ucounts);
+   mutex_destroy(>inotify_data.consumer_mutex);
 }
 
 static void inotify_free_event(struct fsnotify_event *fsn_event)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 498d609..6f5b360 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -230,6 +230,7 @@ static ssize_t inotify_read(struct file *file, char __user 
*buf,
start = buf;
group = file->private_data;
 
+   mutex_lock(>inotify_data.consumer_mutex);
add_wait_queue(>notification_waitq, );
while (1) {
spin_lock(>notification_lock);
@@ -264,6 +265,7 @@ static ssize_t inotify_read(struct file *file, char __user 
*buf,
wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
remove_wait_queue(>notification_waitq, );
+   mutex_unlock(>inotify_data.consumer_mutex);
 
if (start != buf && ret != -EFAULT)
ret = buf - start;
@@ -650,6 +652,8 @@ static struct fsnotify_group *inotify_new_group(unsigned 
int max_events)
 
group->max_events = max_events;
 
+   mutex_init(>inotify_data.consumer_mutex);
+
spin_lock_init(>inotify_data.idr_lock);
idr_init(>inotify_data.idr);
group->inotify_data.ucounts = inc_ucount(current_user_ns(),
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index e6e689b..e0686ed 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -172,6 +172,9 @@ struct fsnotify_group {
spinlock_t  idr_lock;
struct idr  idr;
struct ucounts *ucounts;
+   struct mutexconsumer_mutex; /* Prevent out of order
+* delivery of events
+* to threaded 
consumers */
} inotify_data;
 #endif
 #ifdef CONFIG_FANOTIFY
-- 
2.9.3

[PATCH 0/5] inofify: Reduce lock contention on read()

[Jes Sorensen]

Hi,

Running inotify on a very large number hierachy of directories and
files can result in major lock contention between the producer and
consumer of events.

The current code takes the spinlock when pulling each event off the
queue, which isn't overly idea. This patchset reduces the lock
contention by splicing off the event queue while processing reads and
putting the leftover events back on the queue once the read buffer is
full. In order to ensure ordering when reading events, a mutex is
added to read().

While this should reduce the number of locks taken on read when
reading in bulk, it does increase the overhead for the case where the
users tries to read one event at a time.

Last, the first patch of the series adds the missing mutex_destoy()
for mark_mutex.

Thoughts?
Jes

Jes Sorensen (5):
  notify: Call mutex_destroy() before freeing mutex memory
  inotify: Use mutex to prevent threaded clients reading events out of
order
  notify: Split up some fsnotify functions
  inotify: switch get_one_event() to use fsnotify_list_*() helpers
  inotify: Avoid taking spinlock for each event processed in read()

 fs/notify/group.c|  1 +
 fs/notify/inotify/inotify_fsnotify.c |  1 +
 fs/notify/inotify/inotify_user.c | 42 
 fs/notify/notification.c | 38 ++--
 include/linux/fsnotify_backend.h |  7 ++
 5 files changed, 73 insertions(+), 16 deletions(-)

-- 
2.9.3

[PATCH 2/5] inotify: Use mutex to prevent threaded clients reading events out of order

[Jes Sorensen]

Introduces mutex in the read() path to prevent a threaded client reading
from the same fd consuming events out of order.

This will matter when avoiding taking the spinlock when consuming each
event in the read() path.

Signed-off-by: Jes Sorensen 
Reviewed-by: Josef Bacik 
---
 fs/notify/inotify/inotify_fsnotify.c | 1 +
 fs/notify/inotify/inotify_user.c | 4 
 include/linux/fsnotify_backend.h | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/fs/notify/inotify/inotify_fsnotify.c 
b/fs/notify/inotify/inotify_fsnotify.c
index 1aeb837..63c071f 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -168,6 +168,7 @@ static void inotify_free_group_priv(struct fsnotify_group 
*group)
idr_destroy(>inotify_data.idr);
if (group->inotify_data.ucounts)
dec_inotify_instances(group->inotify_data.ucounts);
+   mutex_destroy(>inotify_data.consumer_mutex);
 }
 
 static void inotify_free_event(struct fsnotify_event *fsn_event)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 498d609..6f5b360 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -230,6 +230,7 @@ static ssize_t inotify_read(struct file *file, char __user 
*buf,
start = buf;
group = file->private_data;
 
+   mutex_lock(>inotify_data.consumer_mutex);
add_wait_queue(>notification_waitq, );
while (1) {
spin_lock(>notification_lock);
@@ -264,6 +265,7 @@ static ssize_t inotify_read(struct file *file, char __user 
*buf,
wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
remove_wait_queue(>notification_waitq, );
+   mutex_unlock(>inotify_data.consumer_mutex);
 
if (start != buf && ret != -EFAULT)
ret = buf - start;
@@ -650,6 +652,8 @@ static struct fsnotify_group *inotify_new_group(unsigned 
int max_events)
 
group->max_events = max_events;
 
+   mutex_init(>inotify_data.consumer_mutex);
+
spin_lock_init(>inotify_data.idr_lock);
idr_init(>inotify_data.idr);
group->inotify_data.ucounts = inc_ucount(current_user_ns(),
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index e6e689b..e0686ed 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -172,6 +172,9 @@ struct fsnotify_group {
spinlock_t  idr_lock;
struct idr  idr;
struct ucounts *ucounts;
+   struct mutexconsumer_mutex; /* Prevent out of order
+* delivery of events
+* to threaded 
consumers */
} inotify_data;
 #endif
 #ifdef CONFIG_FANOTIFY
-- 
2.9.3

[PATCH 0/5] inofify: Reduce lock contention on read()

[Jes Sorensen]

Hi,

Running inotify on a very large number hierachy of directories and
files can result in major lock contention between the producer and
consumer of events.

The current code takes the spinlock when pulling each event off the
queue, which isn't overly idea. This patchset reduces the lock
contention by splicing off the event queue while processing reads and
putting the leftover events back on the queue once the read buffer is
full. In order to ensure ordering when reading events, a mutex is
added to read().

While this should reduce the number of locks taken on read when
reading in bulk, it does increase the overhead for the case where the
users tries to read one event at a time.

Last, the first patch of the series adds the missing mutex_destoy()
for mark_mutex.

Thoughts?
Jes

Jes Sorensen (5):
  notify: Call mutex_destroy() before freeing mutex memory
  inotify: Use mutex to prevent threaded clients reading events out of
order
  notify: Split up some fsnotify functions
  inotify: switch get_one_event() to use fsnotify_list_*() helpers
  inotify: Avoid taking spinlock for each event processed in read()

 fs/notify/group.c|  1 +
 fs/notify/inotify/inotify_fsnotify.c |  1 +
 fs/notify/inotify/inotify_user.c | 42 
 fs/notify/notification.c | 38 ++--
 include/linux/fsnotify_backend.h |  7 ++
 5 files changed, 73 insertions(+), 16 deletions(-)

-- 
2.9.3

Re: [PATCH 1/2] super1: ignore failfast flag for setting device role

[jes . sorensen]

Gioh Kim  writes:
> There is corner case for setting device role,
> if new device has failfast flag.
> The failfast flag should be ignored.
>
> Signed-off-by: Gioh Kim 
> Signed-off-by: Jack Wang 
> ---
>  super1.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/super1.c b/super1.c
> index 882cd61..1da33ef 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1491,6 +1491,7 @@ static int add_to_super1(struct supertype *st, 
> mdu_disk_info_t *dk,
>   struct devinfo *di, **dip;
>   bitmap_super_t *bms = (bitmap_super_t*)(((char*)sb) + MAX_SB_SIZE);
>   int rv, lockid;
> + int dk_state;
>  
>   if (bms->version == BITMAP_MAJOR_CLUSTERED && dlm_funs_ready()) {
>   rv = cluster_get_dlmlock();
> @@ -1501,11 +1502,12 @@ static int add_to_super1(struct supertype *st, 
> mdu_disk_info_t *dk,
>   }
>   }
>  
> - if ((dk->state & 6) == 6) /* active, sync */
> + dk_state &= ~(1< + if ((dk_state & 6) == 6) /* active, sync */
>   *rp = __cpu_to_le16(dk->raid_disk);

This does not look right - you haven't assigned a value to dk_state, but
then start masking bits out of it.

Cheers,
Jes

Re: [PATCH 1/2] super1: ignore failfast flag for setting device role

[jes . sorensen]

Gioh Kim  writes:
> There is corner case for setting device role,
> if new device has failfast flag.
> The failfast flag should be ignored.
>
> Signed-off-by: Gioh Kim 
> Signed-off-by: Jack Wang 
> ---
>  super1.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/super1.c b/super1.c
> index 882cd61..1da33ef 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1491,6 +1491,7 @@ static int add_to_super1(struct supertype *st, 
> mdu_disk_info_t *dk,
>   struct devinfo *di, **dip;
>   bitmap_super_t *bms = (bitmap_super_t*)(((char*)sb) + MAX_SB_SIZE);
>   int rv, lockid;
> + int dk_state;
>  
>   if (bms->version == BITMAP_MAJOR_CLUSTERED && dlm_funs_ready()) {
>   rv = cluster_get_dlmlock();
> @@ -1501,11 +1502,12 @@ static int add_to_super1(struct supertype *st, 
> mdu_disk_info_t *dk,
>   }
>   }
>  
> - if ((dk->state & 6) == 6) /* active, sync */
> + dk_state &= ~(1< + if ((dk_state & 6) == 6) /* active, sync */
>   *rp = __cpu_to_le16(dk->raid_disk);

This does not look right - you haven't assigned a value to dk_state, but
then start masking bits out of it.

Cheers,
Jes

Re: ANNOUNCE: mdadm 4.0 - A tool for managing md Soft RAID under Linux

[Jes Sorensen]

On 02/13/2017 12:54 AM, zhilong wrote:

On 02/13/2017 01:08 PM, zhilong wrote:

Hi, Jes;
On 01/13/2017 12:41 AM, Jes Sorensen wrote:

On 01/11/17 23:24, Guoqing Jiang wrote:


On 01/12/2017 12:59 AM, Jes Sorensen wrote:

On 01/11/17 11:52, Shaohua Li wrote:

On Tue, Jan 10, 2017 at 11:49:04AM -0600, Bruce Dubbs wrote:

Jes Sorensen wrote:

I am pleased to announce the availability of
  mdadm version 4.0

It is available at the usual places:
http://www.kernel.org/pub/linux/utils/raid/mdadm/
and via git at
git://git.kernel.org/pub/scm/utils/mdadm/mdadm.git
  http://git.kernel.org/cgit/utils/mdadm/

The update in major version number primarily indicates this is a
release by it's new maintainer. In addition it contains a large
number
of fixes in particular for IMSM RAID and clustered RAID
support.  In
addition this release includes support for IMSM 4k sector drives,
failfast and better documentation for journaled RAID.

Thank you for the new release.  Unfortunately I get 9 failures
running the
test suite:

tests/00raid1...  FAILED
tests/07autoassemble...   FAILED
tests/07changelevels...   FAILED
tests/07revert-grow...FAILED
tests/07revert-inplace... FAILED
tests/07testreshape5...   FAILED
tests/10ddf-fail-twice... FAILED
tests/20raid5journal...   FAILED
tests/10ddf-incremental-wrong-order...  FAILED

Yep, several tests usually fail. It appears some checks aren't always
good.  At
least the 'check' function for reshape/resync isn't reliable in my
test, I saw
07changelevelintr fails frequently.

That is my experience as well - some of them are affected by the
kernel
version too. We probably need to look into making them more reliable.

If possible, it could be a potential topic for lsf/mm raid
discussion as
Coly suggested
in previous mail.

Is current test can run the test for different raid level, say, "./test
--raidtype=raid1" could
execute all the *r1* tests, does it make sense to do it if we don't
support it now.

We could have a discussion about this at LSF/MM, if someone is willing
to sponsor getting it accepted and we can get the right people there.

Note that the test suite also allows you to run all the 01 tests by
specifying ./test 01. I do like to see the test suite improved and made
more resilient.

I'm sorry for my late response, I'm just back to work today from
vacation. In the past months, I learned and worked for cluster-md
feature,
and I have draft one test suit for cluster-md feature. please refer to
https://github.com/zhilongliu/clustermd-autotest
I'm very willing to do something for improving mdadm testing part,
also wanna improve cluster-md test suit, welcome all comments for it.


  I would keep making cluster-md test scripts more and more stable, and
finally apply to integrate into mdadm test part. :-)


I'd very much like to see work to improve the test suite, so that is great.

Once you have the test suites ready, please post patches and I shall be 
happy to implement them.


Please make sure to test that they don't break if people haven't built 
cluster support into their kernels.


Cheers,
Jes

Re: ANNOUNCE: mdadm 4.0 - A tool for managing md Soft RAID under Linux

[Jes Sorensen]

On 02/13/2017 12:54 AM, zhilong wrote:

On 02/13/2017 01:08 PM, zhilong wrote:

Hi, Jes;
On 01/13/2017 12:41 AM, Jes Sorensen wrote:

On 01/11/17 23:24, Guoqing Jiang wrote:


On 01/12/2017 12:59 AM, Jes Sorensen wrote:

On 01/11/17 11:52, Shaohua Li wrote:

On Tue, Jan 10, 2017 at 11:49:04AM -0600, Bruce Dubbs wrote:

Jes Sorensen wrote:

I am pleased to announce the availability of
  mdadm version 4.0

It is available at the usual places:
http://www.kernel.org/pub/linux/utils/raid/mdadm/
and via git at
git://git.kernel.org/pub/scm/utils/mdadm/mdadm.git
  http://git.kernel.org/cgit/utils/mdadm/

The update in major version number primarily indicates this is a
release by it's new maintainer. In addition it contains a large
number
of fixes in particular for IMSM RAID and clustered RAID
support.  In
addition this release includes support for IMSM 4k sector drives,
failfast and better documentation for journaled RAID.

Thank you for the new release.  Unfortunately I get 9 failures
running the
test suite:

tests/00raid1...  FAILED
tests/07autoassemble...   FAILED
tests/07changelevels...   FAILED
tests/07revert-grow...FAILED
tests/07revert-inplace... FAILED
tests/07testreshape5...   FAILED
tests/10ddf-fail-twice... FAILED
tests/20raid5journal...   FAILED
tests/10ddf-incremental-wrong-order...  FAILED

Yep, several tests usually fail. It appears some checks aren't always
good.  At
least the 'check' function for reshape/resync isn't reliable in my
test, I saw
07changelevelintr fails frequently.

That is my experience as well - some of them are affected by the
kernel
version too. We probably need to look into making them more reliable.

If possible, it could be a potential topic for lsf/mm raid
discussion as
Coly suggested
in previous mail.

Is current test can run the test for different raid level, say, "./test
--raidtype=raid1" could
execute all the *r1* tests, does it make sense to do it if we don't
support it now.

We could have a discussion about this at LSF/MM, if someone is willing
to sponsor getting it accepted and we can get the right people there.

Note that the test suite also allows you to run all the 01 tests by
specifying ./test 01. I do like to see the test suite improved and made
more resilient.

I'm sorry for my late response, I'm just back to work today from
vacation. In the past months, I learned and worked for cluster-md
feature,
and I have draft one test suit for cluster-md feature. please refer to
https://github.com/zhilongliu/clustermd-autotest
I'm very willing to do something for improving mdadm testing part,
also wanna improve cluster-md test suit, welcome all comments for it.


  I would keep making cluster-md test scripts more and more stable, and
finally apply to integrate into mdadm test part. :-)


I'd very much like to see work to improve the test suite, so that is great.

Once you have the test suites ready, please post patches and I shall be 
happy to implement them.


Please make sure to test that they don't break if people haven't built 
cluster support into their kernels.


Cheers,
Jes

Re: ANNOUNCE: mdadm 4.0 - A tool for managing md Soft RAID under Linux

[Jes Sorensen]

On 01/11/17 23:24, Guoqing Jiang wrote:
> 
> 
> On 01/12/2017 12:59 AM, Jes Sorensen wrote:
>> On 01/11/17 11:52, Shaohua Li wrote:
>>> On Tue, Jan 10, 2017 at 11:49:04AM -0600, Bruce Dubbs wrote:
>>>> Jes Sorensen wrote:
>>>>> I am pleased to announce the availability of
>>>>>  mdadm version 4.0
>>>>>
>>>>> It is available at the usual places:
>>>>>  http://www.kernel.org/pub/linux/utils/raid/mdadm/
>>>>> and via git at
>>>>>  git://git.kernel.org/pub/scm/utils/mdadm/mdadm.git
>>>>>  http://git.kernel.org/cgit/utils/mdadm/
>>>>>
>>>>> The update in major version number primarily indicates this is a
>>>>> release by it's new maintainer. In addition it contains a large number
>>>>> of fixes in particular for IMSM RAID and clustered RAID support.  In
>>>>> addition this release includes support for IMSM 4k sector drives,
>>>>> failfast and better documentation for journaled RAID.
>>>> Thank you for the new release.  Unfortunately I get 9 failures
>>>> running the
>>>> test suite:
>>>>
>>>> tests/00raid1...  FAILED
>>>> tests/07autoassemble...   FAILED
>>>> tests/07changelevels...   FAILED
>>>> tests/07revert-grow...FAILED
>>>> tests/07revert-inplace... FAILED
>>>> tests/07testreshape5...   FAILED
>>>> tests/10ddf-fail-twice... FAILED
>>>> tests/20raid5journal...   FAILED
>>>> tests/10ddf-incremental-wrong-order...  FAILED
>>> Yep, several tests usually fail. It appears some checks aren't always
>>> good.  At
>>> least the 'check' function for reshape/resync isn't reliable in my
>>> test, I saw
>>> 07changelevelintr fails frequently.
>> That is my experience as well - some of them are affected by the kernel
>> version too. We probably need to look into making them more reliable.
> 
> If possible, it could be a potential topic for lsf/mm raid discussion as
> Coly suggested
> in previous mail.
> 
> Is current test can run the test for different raid level, say, "./test
> --raidtype=raid1" could
> execute all the *r1* tests, does it make sense to do it if we don't
> support it now.

We could have a discussion about this at LSF/MM, if someone is willing
to sponsor getting it accepted and we can get the right people there.

Note that the test suite also allows you to run all the 01 tests by
specifying ./test 01. I do like to see the test suite improved and made
more resilient.

Cheers,
Jes

Re: ANNOUNCE: mdadm 4.0 - A tool for managing md Soft RAID under Linux

[Jes Sorensen]

On 01/11/17 23:24, Guoqing Jiang wrote:
> 
> 
> On 01/12/2017 12:59 AM, Jes Sorensen wrote:
>> On 01/11/17 11:52, Shaohua Li wrote:
>>> On Tue, Jan 10, 2017 at 11:49:04AM -0600, Bruce Dubbs wrote:
>>>> Jes Sorensen wrote:
>>>>> I am pleased to announce the availability of
>>>>>  mdadm version 4.0
>>>>>
>>>>> It is available at the usual places:
>>>>>  http://www.kernel.org/pub/linux/utils/raid/mdadm/
>>>>> and via git at
>>>>>  git://git.kernel.org/pub/scm/utils/mdadm/mdadm.git
>>>>>  http://git.kernel.org/cgit/utils/mdadm/
>>>>>
>>>>> The update in major version number primarily indicates this is a
>>>>> release by it's new maintainer. In addition it contains a large number
>>>>> of fixes in particular for IMSM RAID and clustered RAID support.  In
>>>>> addition this release includes support for IMSM 4k sector drives,
>>>>> failfast and better documentation for journaled RAID.
>>>> Thank you for the new release.  Unfortunately I get 9 failures
>>>> running the
>>>> test suite:
>>>>
>>>> tests/00raid1...  FAILED
>>>> tests/07autoassemble...   FAILED
>>>> tests/07changelevels...   FAILED
>>>> tests/07revert-grow...FAILED
>>>> tests/07revert-inplace... FAILED
>>>> tests/07testreshape5...   FAILED
>>>> tests/10ddf-fail-twice... FAILED
>>>> tests/20raid5journal...   FAILED
>>>> tests/10ddf-incremental-wrong-order...  FAILED
>>> Yep, several tests usually fail. It appears some checks aren't always
>>> good.  At
>>> least the 'check' function for reshape/resync isn't reliable in my
>>> test, I saw
>>> 07changelevelintr fails frequently.
>> That is my experience as well - some of them are affected by the kernel
>> version too. We probably need to look into making them more reliable.
> 
> If possible, it could be a potential topic for lsf/mm raid discussion as
> Coly suggested
> in previous mail.
> 
> Is current test can run the test for different raid level, say, "./test
> --raidtype=raid1" could
> execute all the *r1* tests, does it make sense to do it if we don't
> support it now.

We could have a discussion about this at LSF/MM, if someone is willing
to sponsor getting it accepted and we can get the right people there.

Note that the test suite also allows you to run all the 01 tests by
specifying ./test 01. I do like to see the test suite improved and made
more resilient.

Cheers,
Jes

Re: ANNOUNCE: mdadm 4.0 - A tool for managing md Soft RAID under Linux

[Jes Sorensen]

On 01/11/17 11:52, Shaohua Li wrote:
> On Tue, Jan 10, 2017 at 11:49:04AM -0600, Bruce Dubbs wrote:
>> Jes Sorensen wrote:
>>> I am pleased to announce the availability of
>>> mdadm version 4.0
>>>
>>> It is available at the usual places:
>>> http://www.kernel.org/pub/linux/utils/raid/mdadm/
>>> and via git at
>>> git://git.kernel.org/pub/scm/utils/mdadm/mdadm.git
>>> http://git.kernel.org/cgit/utils/mdadm/
>>>
>>> The update in major version number primarily indicates this is a
>>> release by it's new maintainer. In addition it contains a large number
>>> of fixes in particular for IMSM RAID and clustered RAID support.  In
>>> addition this release includes support for IMSM 4k sector drives,
>>> failfast and better documentation for journaled RAID.
>>
>> Thank you for the new release.  Unfortunately I get 9 failures running the
>> test suite:
>>
>> tests/00raid1...  FAILED
>> tests/07autoassemble...   FAILED
>> tests/07changelevels...   FAILED
>> tests/07revert-grow...FAILED
>> tests/07revert-inplace... FAILED
>> tests/07testreshape5...   FAILED
>> tests/10ddf-fail-twice... FAILED
>> tests/20raid5journal...   FAILED
>> tests/10ddf-incremental-wrong-order...  FAILED
> 
> Yep, several tests usually fail. It appears some checks aren't always good.  
> At
> least the 'check' function for reshape/resync isn't reliable in my test, I saw
> 07changelevelintr fails frequently.

That is my experience as well - some of them are affected by the kernel
version too. We probably need to look into making them more reliable.

I am also not sure how reliable the DDF tests are on systems without DDF
support.

Cheers,
Jes

Re: ANNOUNCE: mdadm 4.0 - A tool for managing md Soft RAID under Linux

[Jes Sorensen]

On 01/11/17 11:52, Shaohua Li wrote:
> On Tue, Jan 10, 2017 at 11:49:04AM -0600, Bruce Dubbs wrote:
>> Jes Sorensen wrote:
>>> I am pleased to announce the availability of
>>> mdadm version 4.0
>>>
>>> It is available at the usual places:
>>> http://www.kernel.org/pub/linux/utils/raid/mdadm/
>>> and via git at
>>> git://git.kernel.org/pub/scm/utils/mdadm/mdadm.git
>>> http://git.kernel.org/cgit/utils/mdadm/
>>>
>>> The update in major version number primarily indicates this is a
>>> release by it's new maintainer. In addition it contains a large number
>>> of fixes in particular for IMSM RAID and clustered RAID support.  In
>>> addition this release includes support for IMSM 4k sector drives,
>>> failfast and better documentation for journaled RAID.
>>
>> Thank you for the new release.  Unfortunately I get 9 failures running the
>> test suite:
>>
>> tests/00raid1...  FAILED
>> tests/07autoassemble...   FAILED
>> tests/07changelevels...   FAILED
>> tests/07revert-grow...FAILED
>> tests/07revert-inplace... FAILED
>> tests/07testreshape5...   FAILED
>> tests/10ddf-fail-twice... FAILED
>> tests/20raid5journal...   FAILED
>> tests/10ddf-incremental-wrong-order...  FAILED
> 
> Yep, several tests usually fail. It appears some checks aren't always good.  
> At
> least the 'check' function for reshape/resync isn't reliable in my test, I saw
> 07changelevelintr fails frequently.

That is my experience as well - some of them are affected by the kernel
version too. We probably need to look into making them more reliable.

I am also not sure how reliable the DDF tests are on systems without DDF
support.

Cheers,
Jes

ANNOUNCE: mdadm 4.0 - A tool for managing md Soft RAID under Linux

[Jes Sorensen]

I am pleased to announce the availability of
   mdadm version 4.0

It is available at the usual places:
   http://www.kernel.org/pub/linux/utils/raid/mdadm/
and via git at
   git://git.kernel.org/pub/scm/utils/mdadm/mdadm.git
   http://git.kernel.org/cgit/utils/mdadm/

The update in major version number primarily indicates this is a
release by it's new maintainer. In addition it contains a large number
of fixes in particular for IMSM RAID and clustered RAID support.  In
addition this release includes support for IMSM 4k sector drives,
failfast and better documentation for journaled RAID.

This is my first release of mdadm. Please thank Neil Brown for his
previous work as maintainer and blame me for all the bugs I caused
since taking over.

Jes Sorensen, 2017-01-09

ANNOUNCE: mdadm 4.0 - A tool for managing md Soft RAID under Linux

[Jes Sorensen]

I am pleased to announce the availability of
   mdadm version 4.0

It is available at the usual places:
   http://www.kernel.org/pub/linux/utils/raid/mdadm/
and via git at
   git://git.kernel.org/pub/scm/utils/mdadm/mdadm.git
   http://git.kernel.org/cgit/utils/mdadm/

The update in major version number primarily indicates this is a
release by it's new maintainer. In addition it contains a large number
of fixes in particular for IMSM RAID and clustered RAID support.  In
addition this release includes support for IMSM 4k sector drives,
failfast and better documentation for journaled RAID.

This is my first release of mdadm. Please thank Neil Brown for his
previous work as maintainer and blame me for all the bugs I caused
since taking over.

Jes Sorensen, 2017-01-09

Re: PROBLEM: Kernel BUG with raid5 soft + Xen + DRBD - invalid opcode

[Jes Sorensen]

MasterPrenium  writes:
> Hello Guys,
>
> I've having some trouble on a new system I'm setting up. I'm getting a
> kernel BUG message, seems to be related with the use of Xen (when I
> boot the system _without_ Xen, I don't get any crash).
> Here is configuration :
> - 3x Hard Drives running on RAID 5 Software raid created by mdadm
> - On top of it, DRBD for replication over another node (Active/passive 
> cluster)
> - On top of it, a BTRFS FileSystem with a few subvolumes
> - On top of it, XEN VMs running.
>
> The BUG is happening when I'm making "huge" I/O (20MB/s with a rsync
> for example) on the RAID5 stack.
> I've to reset system to make it work again.
>
> Reproducible : ALWAYS (making the i/o, it crash in 2-5mins). Also
> reproducible on another system with the same hardware.
>
> Kernel versions impacted (at least): kernel-4.4.26, kernel-4.8.15, 
> kernel-4.9.0

Well you have one foreign object in there that is not part of the
kernel and which shows up in the OOPS: DRDB

What happens when you remove that from the equation?

Jes

Re: PROBLEM: Kernel BUG with raid5 soft + Xen + DRBD - invalid opcode

[Jes Sorensen]

MasterPrenium  writes:
> Hello Guys,
>
> I've having some trouble on a new system I'm setting up. I'm getting a
> kernel BUG message, seems to be related with the use of Xen (when I
> boot the system _without_ Xen, I don't get any crash).
> Here is configuration :
> - 3x Hard Drives running on RAID 5 Software raid created by mdadm
> - On top of it, DRBD for replication over another node (Active/passive 
> cluster)
> - On top of it, a BTRFS FileSystem with a few subvolumes
> - On top of it, XEN VMs running.
>
> The BUG is happening when I'm making "huge" I/O (20MB/s with a rsync
> for example) on the RAID5 stack.
> I've to reset system to make it work again.
>
> Reproducible : ALWAYS (making the i/o, it crash in 2-5mins). Also
> reproducible on another system with the same hardware.
>
> Kernel versions impacted (at least): kernel-4.4.26, kernel-4.8.15, 
> kernel-4.9.0

Well you have one foreign object in there that is not part of the
kernel and which shows up in the OOPS: DRDB

What happens when you remove that from the equation?

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] 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 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: [mdadm PATCH] Add failfast support.

[Jes Sorensen]

NeilBrown  writes:
> Allow per-device "failfast" flag to be set when creating an
> array or adding devices to an array.
>
> When re-adding a device which had the failfast flag, it can be removed
> using --nofailfast.
>
> failfast status is printed in --detail and --examine output.
>
> Signed-off-by: NeilBrown 
> ---
>
> Hi Jes,
>  this patch adds mdadm support for the failfast functionality that
> Shaohua recently included in his for-next.
> Hopefully the man-page additions provide all necessary context.
> If there is anything that seems to be missing, I'll be very happy to
> add it.

Hi Neil,

It looks reasonable. The only minor concern I have is over the use of
hardcoded magic numbers like 'dv->failfast = 2' rather than using an
enum or defines with descriptive names. Something that can be addressed
later, so patch applied.

Cheers,
Jes

Re: [mdadm PATCH] Add failfast support.

[Jes Sorensen]

NeilBrown  writes:
> Allow per-device "failfast" flag to be set when creating an
> array or adding devices to an array.
>
> When re-adding a device which had the failfast flag, it can be removed
> using --nofailfast.
>
> failfast status is printed in --detail and --examine output.
>
> Signed-off-by: NeilBrown 
> ---
>
> Hi Jes,
>  this patch adds mdadm support for the failfast functionality that
> Shaohua recently included in his for-next.
> Hopefully the man-page additions provide all necessary context.
> If there is anything that seems to be missing, I'll be very happy to
> add it.

Hi Neil,

It looks reasonable. The only minor concern I have is over the use of
hardcoded magic numbers like 'dv->failfast = 2' rather than using an
enum or defines with descriptive names. Something that can be addressed
later, so patch applied.

Cheers,
Jes