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, j...@posteo.de 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;
>>
