unhandled firmware response in iwm

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

unhandled firmware response in iwm

jes@posteo.de
When connecting to a wifi network messages like "iwm0: unhandled
firmware response 0xff/0xb8000010 rx ring" appear.
After looking at https://patchwork.kernel.org/patch/9869017/ I came up
with following patch.
The link also explains what is happening.

I'm not sure if it makes sense to log the firmware responses like the
linux kernel does.
They don't appear to have any negative effect so I decided against it.

diff --git sys/dev/pci/if_iwm.c sys/dev/pci/if_iwm.c
index 476c61f0b3d..77d87fa079e 100644
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -7219,6 +7219,8 @@ iwm_notif_intr(struct iwm_softc *sc)
                 case IWM_WIDE_ID(IWM_ALWAYS_LONG_GROUP,
IWM_SCAN_CFG_CMD):
                 case IWM_WIDE_ID(IWM_ALWAYS_LONG_GROUP,
IWM_SCAN_REQ_UMAC):
                 case IWM_WIDE_ID(IWM_ALWAYS_LONG_GROUP,
IWM_SCAN_ABORT_UMAC):
+               case IWM_WIDE_ID(IWM_SYSTEM_GROUP,
+                   IWM_FSEQ_VER_MISMATCH_NOTIFICATION):
                 case IWM_SCAN_OFFLOAD_REQUEST_CMD:
                 case IWM_SCAN_OFFLOAD_ABORT_CMD:
                 case IWM_REPLY_BEACON_FILTERING_CMD:
diff --git sys/dev/pci/if_iwmreg.h sys/dev/pci/if_iwmreg.h
index 3c5cfea7b23..06eef017015 100644
--- sys/dev/pci/if_iwmreg.h
+++ sys/dev/pci/if_iwmreg.h
@@ -1813,6 +1813,9 @@ struct iwm_agn_scd_bc_tbl {
  #define IWM_NET_DETECT_HOTSPOTS_CMD            0x58
  #define IWM_NET_DETECT_HOTSPOTS_QUERY_CMD      0x59

+/* system group command IDs */
+#define IWM_FSEQ_VER_MISMATCH_NOTIFICATION     0xff
+
  #define IWM_REPLY_MAX  0xff

  /**
@@ -5820,6 +5823,7 @@ iwm_cmd_id(uint8_t opcode, uint8_t groupid,
uint8_t version)

  /* due to the conversion, this group is special */
  #define IWM_ALWAYS_LONG_GROUP  1
+#define IWM_SYSTEM_GROUP       4

  struct iwm_cmd_header {
         uint8_t code;

Reply | Threaded
Open this post in threaded view
|

Re: unhandled firmware response in iwm

Stefan Sperling-5
On Mon, Feb 26, 2018 at 11:55:34PM +0100, [hidden email] wrote:
> When connecting to a wifi network messages like "iwm0: unhandled firmware
> response 0xff/0xb8000010 rx ring" appear.
> After looking at https://patchwork.kernel.org/patch/9869017/ I came up with
> following patch.
> The link also explains what is happening.

Thanks for this!

> I'm not sure if it makes sense to log the firmware responses like the linux
> kernel does.
> They don't appear to have any negative effect so I decided against it.

Agreed. There is no point in logging this.

There are two problems with your diff:

The first is that somehow it got corrupted (whitespace got mangled, lines
got wrapped which shouldn't have been). Please check your mailer settings.
Tip: You can mail a diff to yourself and then try to apply it until you've
figured out how to make your mail client work right. I've quoted the diff
I received below as-is, so if you save this mail body and feed it into
patch(1) you will see what problems I saw.
In this particular instance it's not a huge burden because the diff adds
just 3 lines, but I basically had to rewrite your patch from scratch to
apply it, which is no fun, especially for larger diffs.

The next problem: You're catching this notification in a case block which
handles commands we expect to get a response for, but we don't even want
to parse the data contained in this notification.
I think it would be better to move it to a different case: block which just
does a 'break;'. There should be some of those already so you could add it
there, or you could introduce a new case: block, whichever you prefer.

diff --git sys/dev/pci/if_iwm.c sys/dev/pci/if_iwm.c
index 476c61f0b3d..77d87fa079e 100644
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -7219,6 +7219,8 @@ iwm_notif_intr(struct iwm_softc *sc)
                case IWM_WIDE_ID(IWM_ALWAYS_LONG_GROUP, IWM_SCAN_CFG_CMD):
                case IWM_WIDE_ID(IWM_ALWAYS_LONG_GROUP, IWM_SCAN_REQ_UMAC):
                case IWM_WIDE_ID(IWM_ALWAYS_LONG_GROUP,
IWM_SCAN_ABORT_UMAC):
+               case IWM_WIDE_ID(IWM_SYSTEM_GROUP,
+                   IWM_FSEQ_VER_MISMATCH_NOTIFICATION):
                case IWM_SCAN_OFFLOAD_REQUEST_CMD:
                case IWM_SCAN_OFFLOAD_ABORT_CMD:
                case IWM_REPLY_BEACON_FILTERING_CMD:
diff --git sys/dev/pci/if_iwmreg.h sys/dev/pci/if_iwmreg.h
index 3c5cfea7b23..06eef017015 100644
--- sys/dev/pci/if_iwmreg.h
+++ sys/dev/pci/if_iwmreg.h
@@ -1813,6 +1813,9 @@ struct iwm_agn_scd_bc_tbl {
 #define IWM_NET_DETECT_HOTSPOTS_CMD            0x58
 #define IWM_NET_DETECT_HOTSPOTS_QUERY_CMD      0x59

+/* system group command IDs */
+#define IWM_FSEQ_VER_MISMATCH_NOTIFICATION     0xff
+
 #define IWM_REPLY_MAX  0xff

 /**
@@ -5820,6 +5823,7 @@ iwm_cmd_id(uint8_t opcode, uint8_t groupid, uint8_t
version)

 /* due to the conversion, this group is special */
 #define IWM_ALWAYS_LONG_GROUP  1
+#define IWM_SYSTEM_GROUP       4

 struct iwm_cmd_header {
        uint8_t code;

Reply | Threaded
Open this post in threaded view
|

Re: unhandled firmware response in iwm

jes@posteo.de
On 02/27/18 09:06, Stefan Sperling wrote:

> On Mon, Feb 26, 2018 at 11:55:34PM +0100, [hidden email] wrote:
>> When connecting to a wifi network messages like "iwm0: unhandled firmware
>> response 0xff/0xb8000010 rx ring" appear.
>> After looking at https://patchwork.kernel.org/patch/9869017/ I came up with
>> following patch.
>> The link also explains what is happening.
>
> Thanks for this!
>
>> I'm not sure if it makes sense to log the firmware responses like the linux
>> kernel does.
>> They don't appear to have any negative effect so I decided against it.
>
> Agreed. There is no point in logging this.
>
> There are two problems with your diff:
>
> The first is that somehow it got corrupted (whitespace got mangled, lines
> got wrapped which shouldn't have been). Please check your mailer settings.
> Tip: You can mail a diff to yourself and then try to apply it until you've
> figured out how to make your mail client work right. I've quoted the diff
> I received below as-is, so if you save this mail body and feed it into
> patch(1) you will see what problems I saw.
> In this particular instance it's not a huge burden because the diff adds
> just 3 lines, but I basically had to rewrite your patch from scratch to
> apply it, which is no fun, especially for larger diffs.

Sorry for that. Seems my config was eaten. Again.

> The next problem: You're catching this notification in a case block which
> handles commands we expect to get a response for, but we don't even want
> to parse the data contained in this notification.
> I think it would be better to move it to a different case: block which just
> does a 'break;'. There should be some of those already so you could add it
> there, or you could introduce a new case: block, whichever you prefer.

I moved the catch which is marked as ignored. Thanks for the review!

diff --git sys/dev/pci/if_iwm.c sys/dev/pci/if_iwm.c
index 476c61f0b3d..af3eeb76978 100644
--- sys/dev/pci/if_iwm.c
+++ sys/dev/pci/if_iwm.c
@@ -7316,6 +7316,10 @@ iwm_notif_intr(struct iwm_softc *sc)
                        break;
                }
 
+               case IWM_WIDE_ID(IWM_SYSTEM_GROUP,
+                   IWM_FSEQ_VER_MISMATCH_NOTIFICATION):
+                       break;
+
                /*
                 * Firmware versions 21 and 22 generate some DEBUG_LOG_MSG
                 * messages. Just ignore them for now.
diff --git sys/dev/pci/if_iwmreg.h sys/dev/pci/if_iwmreg.h
index 3c5cfea7b23..06eef017015 100644
--- sys/dev/pci/if_iwmreg.h
+++ sys/dev/pci/if_iwmreg.h
@@ -1813,6 +1813,9 @@ struct iwm_agn_scd_bc_tbl {
 #define IWM_NET_DETECT_HOTSPOTS_CMD            0x58
 #define IWM_NET_DETECT_HOTSPOTS_QUERY_CMD      0x59
 
+/* system group command IDs */
+#define IWM_FSEQ_VER_MISMATCH_NOTIFICATION     0xff
+
 #define IWM_REPLY_MAX  0xff
 
 /**
@@ -5820,6 +5823,7 @@ iwm_cmd_id(uint8_t opcode, uint8_t groupid, uint8_t version)
 
 /* due to the conversion, this group is special */
 #define IWM_ALWAYS_LONG_GROUP  1
+#define IWM_SYSTEM_GROUP       4
 
 struct iwm_cmd_header {
        uint8_t code;

Reply | Threaded
Open this post in threaded view
|

Re: unhandled firmware response in iwm

Stefan Sperling-5
On Tue, Feb 27, 2018 at 10:55:05AM +0100, Jan Schreiber wrote:

> On 02/27/18 09:06, Stefan Sperling wrote:
> > On Mon, Feb 26, 2018 at 11:55:34PM +0100, [hidden email] wrote:
> >> When connecting to a wifi network messages like "iwm0: unhandled firmware
> >> response 0xff/0xb8000010 rx ring" appear.
> >> After looking at https://patchwork.kernel.org/patch/9869017/ I came up with
> >> following patch.
> >> The link also explains what is happening.
> >
> > Thanks for this!
> >
> >> I'm not sure if it makes sense to log the firmware responses like the linux
> >> kernel does.
> >> They don't appear to have any negative effect so I decided against it.
> >
> > Agreed. There is no point in logging this.
> >
> > There are two problems with your diff:
> >
> > The first is that somehow it got corrupted (whitespace got mangled, lines
> > got wrapped which shouldn't have been). Please check your mailer settings.
> > Tip: You can mail a diff to yourself and then try to apply it until you've
> > figured out how to make your mail client work right. I've quoted the diff
> > I received below as-is, so if you save this mail body and feed it into
> > patch(1) you will see what problems I saw.
> > In this particular instance it's not a huge burden because the diff adds
> > just 3 lines, but I basically had to rewrite your patch from scratch to
> > apply it, which is no fun, especially for larger diffs.
>
> Sorry for that. Seems my config was eaten. Again.

This new diff is still broken (looks like tabs got converted to spaces)?
You obviously didn't try to send a diff to yourself first :(

I am just going to apply it manually now. But please invest time into
fixing this properly before submitting any more diffs.
For every diff you send out there is a potential amount of N people who will
try to test it. Problems like this are just distracting and a waste of time
for anyone volunteering some of their own time to try to help you.

Regardless, I am very grateful for the work you did in tracking this down.
This has been a long-standing problem and you've provided the missing link
to information which allowed us to understand why it happened.

|diff --git sys/dev/pci/if_iwm.c sys/dev/pci/if_iwm.c
|index 476c61f0b3d..af3eeb76978 100644
|--- sys/dev/pci/if_iwm.c
|+++ sys/dev/pci/if_iwm.c
--------------------------
Patching file if_iwm.c using Plan A...
Hunk #1 failed at 7316.
1 out of 1 hunks failed--saving rejects to if_iwm.c.rej
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git sys/dev/pci/if_iwmreg.h sys/dev/pci/if_iwmreg.h
|index 3c5cfea7b23..06eef017015 100644
|--- sys/dev/pci/if_iwmreg.h
|+++ sys/dev/pci/if_iwmreg.h
--------------------------
Patching file if_iwmreg.h using Plan A...
Hunk #1 failed at 1813.
Hunk #2 failed at 5823.
2 out of 2 hunks failed--saving rejects to if_iwmreg.h.rej
Hmm...  Ignoring the trailing garbage.
done


> > The next problem: You're catching this notification in a case block which
> > handles commands we expect to get a response for, but we don't even want
> > to parse the data contained in this notification.
> > I think it would be better to move it to a different case: block which just
> > does a 'break;'. There should be some of those already so you could add it
> > there, or you could introduce a new case: block, whichever you prefer.
>
> I moved the catch which is marked as ignored. Thanks for the review!
>
> diff --git sys/dev/pci/if_iwm.c sys/dev/pci/if_iwm.c
> index 476c61f0b3d..af3eeb76978 100644
> --- sys/dev/pci/if_iwm.c
> +++ sys/dev/pci/if_iwm.c
> @@ -7316,6 +7316,10 @@ iwm_notif_intr(struct iwm_softc *sc)
>                         break;
>                 }
>  
> +               case IWM_WIDE_ID(IWM_SYSTEM_GROUP,
> +                   IWM_FSEQ_VER_MISMATCH_NOTIFICATION):
> +                       break;
> +
>                 /*
>                  * Firmware versions 21 and 22 generate some DEBUG_LOG_MSG
>                  * messages. Just ignore them for now.
> diff --git sys/dev/pci/if_iwmreg.h sys/dev/pci/if_iwmreg.h
> index 3c5cfea7b23..06eef017015 100644
> --- sys/dev/pci/if_iwmreg.h
> +++ sys/dev/pci/if_iwmreg.h
> @@ -1813,6 +1813,9 @@ struct iwm_agn_scd_bc_tbl {
>  #define IWM_NET_DETECT_HOTSPOTS_CMD            0x58
>  #define IWM_NET_DETECT_HOTSPOTS_QUERY_CMD      0x59
>  
> +/* system group command IDs */
> +#define IWM_FSEQ_VER_MISMATCH_NOTIFICATION     0xff
> +
>  #define IWM_REPLY_MAX  0xff
>  
>  /**
> @@ -5820,6 +5823,7 @@ iwm_cmd_id(uint8_t opcode, uint8_t groupid, uint8_t version)
>  
>  /* due to the conversion, this group is special */
>  #define IWM_ALWAYS_LONG_GROUP  1
> +#define IWM_SYSTEM_GROUP       4
>  
>  struct iwm_cmd_header {
>         uint8_t code;
>

Reply | Threaded
Open this post in threaded view
|

Re: unhandled firmware response in iwm

jes@posteo.de
On 02/28/18 15:35, Stefan Sperling wrote:

> On Tue, Feb 27, 2018 at 10:55:05AM +0100, Jan Schreiber wrote:
>> On 02/27/18 09:06, Stefan Sperling wrote:
>>> On Mon, Feb 26, 2018 at 11:55:34PM +0100, [hidden email] wrote:
>>>> When connecting to a wifi network messages like "iwm0: unhandled firmware
>>>> response 0xff/0xb8000010 rx ring" appear.
>>>> After looking at https://patchwork.kernel.org/patch/9869017/ I came up with
>>>> following patch.
>>>> The link also explains what is happening.
>>>
>>> Thanks for this!
>>>
>>>> I'm not sure if it makes sense to log the firmware responses like the linux
>>>> kernel does.
>>>> They don't appear to have any negative effect so I decided against it.
>>>
>>> Agreed. There is no point in logging this.
>>>
>>> There are two problems with your diff:
>>>
>>> The first is that somehow it got corrupted (whitespace got mangled, lines
>>> got wrapped which shouldn't have been). Please check your mailer settings.
>>> Tip: You can mail a diff to yourself and then try to apply it until you've
>>> figured out how to make your mail client work right. I've quoted the diff
>>> I received below as-is, so if you save this mail body and feed it into
>>> patch(1) you will see what problems I saw.
>>> In this particular instance it's not a huge burden because the diff adds
>>> just 3 lines, but I basically had to rewrite your patch from scratch to
>>> apply it, which is no fun, especially for larger diffs.
>>
>> Sorry for that. Seems my config was eaten. Again.
>
> This new diff is still broken (looks like tabs got converted to spaces)?
> You obviously didn't try to send a diff to yourself first :(

Sorry for the inconvenience. Thank you for the extra work!

>
> I am just going to apply it manually now. But please invest time into
> fixing this properly before submitting any more diffs.
> For every diff you send out there is a potential amount of N people who will
> try to test it. Problems like this are just distracting and a waste of time
> for anyone volunteering some of their own time to try to help you.
>
> Regardless, I am very grateful for the work you did in tracking this down.
> This has been a long-standing problem and you've provided the missing link
> to information which allowed us to understand why it happened.
>
> |diff --git sys/dev/pci/if_iwm.c sys/dev/pci/if_iwm.c
> |index 476c61f0b3d..af3eeb76978 100644
> |--- sys/dev/pci/if_iwm.c
> |+++ sys/dev/pci/if_iwm.c
> --------------------------
> Patching file if_iwm.c using Plan A...
> Hunk #1 failed at 7316.
> 1 out of 1 hunks failed--saving rejects to if_iwm.c.rej
> Hmm...  The next patch looks like a unified diff to me...
> The text leading up to this was:
> --------------------------
> |diff --git sys/dev/pci/if_iwmreg.h sys/dev/pci/if_iwmreg.h
> |index 3c5cfea7b23..06eef017015 100644
> |--- sys/dev/pci/if_iwmreg.h
> |+++ sys/dev/pci/if_iwmreg.h
> --------------------------
> Patching file if_iwmreg.h using Plan A...
> Hunk #1 failed at 1813.
> Hunk #2 failed at 5823.
> 2 out of 2 hunks failed--saving rejects to if_iwmreg.h.rej
> Hmm...  Ignoring the trailing garbage.
> done
>
>
>>> The next problem: You're catching this notification in a case block which
>>> handles commands we expect to get a response for, but we don't even want
>>> to parse the data contained in this notification.
>>> I think it would be better to move it to a different case: block which just
>>> does a 'break;'. There should be some of those already so you could add it
>>> there, or you could introduce a new case: block, whichever you prefer.
>>
>> I moved the catch which is marked as ignored. Thanks for the review!
>>
>> diff --git sys/dev/pci/if_iwm.c sys/dev/pci/if_iwm.c
>> index 476c61f0b3d..af3eeb76978 100644
>> --- sys/dev/pci/if_iwm.c
>> +++ sys/dev/pci/if_iwm.c
>> @@ -7316,6 +7316,10 @@ iwm_notif_intr(struct iwm_softc *sc)
>>                         break;
>>                 }
>>  
>> +               case IWM_WIDE_ID(IWM_SYSTEM_GROUP,
>> +                   IWM_FSEQ_VER_MISMATCH_NOTIFICATION):
>> +                       break;
>> +
>>                 /*
>>                  * Firmware versions 21 and 22 generate some DEBUG_LOG_MSG
>>                  * messages. Just ignore them for now.
>> diff --git sys/dev/pci/if_iwmreg.h sys/dev/pci/if_iwmreg.h
>> index 3c5cfea7b23..06eef017015 100644
>> --- sys/dev/pci/if_iwmreg.h
>> +++ sys/dev/pci/if_iwmreg.h
>> @@ -1813,6 +1813,9 @@ struct iwm_agn_scd_bc_tbl {
>>  #define IWM_NET_DETECT_HOTSPOTS_CMD            0x58
>>  #define IWM_NET_DETECT_HOTSPOTS_QUERY_CMD      0x59
>>  
>> +/* system group command IDs */
>> +#define IWM_FSEQ_VER_MISMATCH_NOTIFICATION     0xff
>> +
>>  #define IWM_REPLY_MAX  0xff
>>  
>>  /**
>> @@ -5820,6 +5823,7 @@ iwm_cmd_id(uint8_t opcode, uint8_t groupid, uint8_t version)
>>  
>>  /* due to the conversion, this group is special */
>>  #define IWM_ALWAYS_LONG_GROUP  1
>> +#define IWM_SYSTEM_GROUP       4
>>  
>>  struct iwm_cmd_header {
>>         uint8_t code;
>>