Project

General

Profile

Submit #3044

isp: Unify firmware handling with the rest of the system.

Added by sucanjan@fit.cvut.cz 2 months ago. Updated about 2 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
06/12/2017
Due date:
% Done:

100%


Description

Hello,

these patches convert firmware binary data from C arrays in a header files to uuencode .uu files. Byte order of the firmware image is little-endian. As far as DragonFly BSD supports only little-endian architectures it's ok. But in case of a big-endian architecture the byte order must be changed before firmware is processed by the isp driver. They also move the .uu images to a sys/contrib/dev/isp.

The first part concerns these firmwares: isp_1040, isp_1040_it, isp_1080, isp_1080_it, isp_12160, isp_12160_it, isp_2100, isp_2200, isp_2300 and isp_2322.
The second part concerns these firmwares: isp_2400, isp_2400_multi, isp_2500, isp_2500_multi.

The division is needed because a size of a single patch must be less than 5 MB for the bugtracker to accept.

0001-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch View (3.47 MB) sucanjan@fit.cvut.cz, 06/12/2017 09:23 AM

0002-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch View (4.02 MB) sucanjan@fit.cvut.cz, 06/12/2017 09:38 AM

convert.sh View (8.24 KB) sucanjan@fit.cvut.cz, 06/23/2017 03:51 AM

0001-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch View (3.49 MB) sucanjan@fit.cvut.cz, 06/25/2017 05:42 AM

0002-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch View (4.02 MB) sucanjan@fit.cvut.cz, 06/25/2017 05:48 AM

0003-ispfw.4-Use-modules-with-individual-firmwares-instea.patch View (1.46 KB) sucanjan@fit.cvut.cz, 06/28/2017 01:50 AM

0004-isp.4-Modules-with-individual-firmwares-can-be-loade.patch View (1.46 KB) sucanjan@fit.cvut.cz, 06/28/2017 01:50 AM

History

#1 Updated by sepherosa 2 months ago

Let's wait a bit for others' input. I am ok w/ it.

On Tue, Jun 13, 2017 at 12:39 AM,
<> wrote:
> Issue #3044 has been reported by .
>
> ----------------------------------------
> Submit #3044: isp: Unify firmware handling with the rest of the system.
> http://bugs.dragonflybsd.org/issues/3044
>
> * Author:
> * Status: New
> * Priority: Normal
> * Assignee:
> * Category:
> * Target version:
> ----------------------------------------
> Hello,
>
> these patches convert firmware binary data from C arrays in a header files to uuencode .uu files. Byte order of the firmware image is little-endian. As far as DragonFly BSD supports only little-endian architectures it's ok. But in case of a big-endian architecture the byte order must be changed before firmware is processed by the isp driver. They also move the .uu images to a sys/contrib/dev/isp.
>
> The first part concerns these firmwares: isp_1040, isp_1040_it, isp_1080, isp_1080_it, isp_12160, isp_12160_it, isp_2100, isp_2200, isp_2300 and isp_2322.
> The second part concerns these firmwares: isp_2400, isp_2400_multi, isp_2500, isp_2500_multi.
>
> The division is needed because a size of a single patch must be less than 5 MB for the bugtracker to accept.
>
>
> ---Files--------------------------------
> 0001-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (3.47 MB)
> 0002-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (4.02 MB)
>
>
> --
> You have received this notification because you have either subscribed to it, or are involved in it.
> To change your notification preferences, please click here: http://bugs.dragonflybsd.org/my/account

--
Tomorrow Will Never Die

#2 Updated by swildner about 2 months ago

  • Status changed from New to In Progress

Thanks, I've checked the patches and they work on this 1020 card I own (using isp_1040 firmware I suppose).

How exactly did you generate the *.uu files? How would we sync it with FreeBSD's in the future?

#3 Updated by swildner about 2 months ago

  • Assignee set to swildner

#4 Updated by sucanjan@fit.cvut.cz about 2 months ago

swildner wrote:
> Thanks, I've checked the patches and they work on this 1020 card I own (using isp_1040 firmware I suppose).
>
> How exactly did you generate the *.uu files? How would we sync it with FreeBSD's in the future?

I generated the *.uu files using command-line tools. I

- removed all lines from a C header file keeping only those lines with values of an array for one firmware image (e.g. 0x1234, 0x5678, ...)
- transformed the values (swapped the bytes to be in little-endian order) using sed to an arguments for the bash-builtin printf command (printf "\x34\x12"; printf "\x78\x56"; ...)
- executed generated printf calls as a bash script and redirected stdout to a file
- converted the binary file with uuencode
- concatenated the .uu file with a corresponding license text

About the sync with FreeBSD's: the truth is I didn't think about that.

#5 Updated by sepherosa about 2 months ago

It will be great, if this can be automated.

On Tue, Jun 20, 2017 at 4:39 AM,
<> wrote:
> Issue #3044 has been updated by .
>
>
> swildner wrote:
>> Thanks, I've checked the patches and they work on this 1020 card I own (using isp_1040 firmware I suppose).
>>
>> How exactly did you generate the *.uu files? How would we sync it with FreeBSD's in the future?
>
> I generated the *.uu files using command-line tools. I
>
> - removed all lines from a C header file keeping only those lines with values of an array for one firmware image (e.g. 0x1234, 0x5678, ...)
> - transformed the values (swapped the bytes to be in little-endian order) using sed to an arguments for the bash-builtin printf command (printf "\x34\x12"; printf "\x78\x56"; ...)
> - executed generated printf calls as a bash script and redirected stdout to a file
> - converted the binary file with uuencode
> - concatenated the .uu file with a corresponding license text
>
> About the sync with FreeBSD's: the truth is I didn't think about that.
>
> ----------------------------------------
> Submit #3044: isp: Unify firmware handling with the rest of the system.
> http://bugs.dragonflybsd.org/issues/3044#change-13167
>
> * Author:
> * Status: In Progress
> * Priority: Normal
> * Assignee: swildner
> * Category:
> * Target version:
> ----------------------------------------
> Hello,
>
> these patches convert firmware binary data from C arrays in a header files to uuencode .uu files. Byte order of the firmware image is little-endian. As far as DragonFly BSD supports only little-endian architectures it's ok. But in case of a big-endian architecture the byte order must be changed before firmware is processed by the isp driver. They also move the .uu images to a sys/contrib/dev/isp.
>
> The first part concerns these firmwares: isp_1040, isp_1040_it, isp_1080, isp_1080_it, isp_12160, isp_12160_it, isp_2100, isp_2200, isp_2300 and isp_2322.
> The second part concerns these firmwares: isp_2400, isp_2400_multi, isp_2500, isp_2500_multi.
>
> The division is needed because a size of a single patch must be less than 5 MB for the bugtracker to accept.
>
>
> ---Files--------------------------------
> 0001-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (3.47 MB)
> 0002-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (4.02 MB)
>
>
> --
> You have received this notification because you have either subscribed to it, or are involved in it.
> To change your notification preferences, please click here: http://bugs.dragonflybsd.org/my/account

--
Tomorrow Will Never Die

#6 Updated by sucanjan@fit.cvut.cz about 2 months ago

I can create a small tool which will take lines of hex numbers on stdin and print corresponding values to stdout. Could it be this way?

sepherosa wrote:
> It will be great, if this can be automated.
>
> On Tue, Jun 20, 2017 at 4:39 AM,
> <> wrote:
> > Issue #3044 has been updated by .
> >
> >
> > swildner wrote:
> >> Thanks, I've checked the patches and they work on this 1020 card I own (using isp_1040 firmware I suppose).
> >>
> >> How exactly did you generate the *.uu files? How would we sync it with FreeBSD's in the future?
> >
> > I generated the *.uu files using command-line tools. I
> >
> > - removed all lines from a C header file keeping only those lines with values of an array for one firmware image (e.g. 0x1234, 0x5678, ...)
> > - transformed the values (swapped the bytes to be in little-endian order) using sed to an arguments for the bash-builtin printf command (printf "\x34\x12"; printf "\x78\x56"; ...)
> > - executed generated printf calls as a bash script and redirected stdout to a file
> > - converted the binary file with uuencode
> > - concatenated the .uu file with a corresponding license text
> >
> > About the sync with FreeBSD's: the truth is I didn't think about that.
> >
> > ----------------------------------------
> > Submit #3044: isp: Unify firmware handling with the rest of the system.
> > http://bugs.dragonflybsd.org/issues/3044#change-13167
> >
> > * Author:
> > * Status: In Progress
> > * Priority: Normal
> > * Assignee: swildner
> > * Category:
> > * Target version:
> > ----------------------------------------
> > Hello,
> >
> > these patches convert firmware binary data from C arrays in a header files to uuencode .uu files. Byte order of the firmware image is little-endian. As far as DragonFly BSD supports only little-endian architectures it's ok. But in case of a big-endian architecture the byte order must be changed before firmware is processed by the isp driver. They also move the .uu images to a sys/contrib/dev/isp.
> >
> > The first part concerns these firmwares: isp_1040, isp_1040_it, isp_1080, isp_1080_it, isp_12160, isp_12160_it, isp_2100, isp_2200, isp_2300 and isp_2322.
> > The second part concerns these firmwares: isp_2400, isp_2400_multi, isp_2500, isp_2500_multi.
> >
> > The division is needed because a size of a single patch must be less than 5 MB for the bugtracker to accept.
> >
> >
> > ---Files--------------------------------
> > 0001-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (3.47 MB)
> > 0002-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (4.02 MB)
> >
> >
> > --
> > You have received this notification because you have either subscribed to it, or are involved in it.
> > To change your notification preferences, please click here: http://bugs.dragonflybsd.org/my/account
>
>
>
> --
> Tomorrow Will Never Die

#7 Updated by sepherosa about 2 months ago

Hmm, I wanted to mean, the tool could be isp specific. Take an isp
firmware header file, and
convert it to uu file. The tool does not need to be generic; it is
only used by us to sync other BSD's isp firmware.

On Tue, Jun 20, 2017 at 4:07 PM,
<> wrote:
> Issue #3044 has been updated by .
>
>
> I can create a small tool which will take lines of hex numbers on stdin and print corresponding values to stdout. Could it be this way?
>
> sepherosa wrote:
>> It will be great, if this can be automated.
>>
>> On Tue, Jun 20, 2017 at 4:39 AM,
>> <> wrote:
>> > Issue #3044 has been updated by .
>> >
>> >
>> > swildner wrote:
>> >> Thanks, I've checked the patches and they work on this 1020 card I own (using isp_1040 firmware I suppose).
>> >>
>> >> How exactly did you generate the *.uu files? How would we sync it with FreeBSD's in the future?
>> >
>> > I generated the *.uu files using command-line tools. I
>> >
>> > - removed all lines from a C header file keeping only those lines with values of an array for one firmware image (e.g. 0x1234, 0x5678, ...)
>> > - transformed the values (swapped the bytes to be in little-endian order) using sed to an arguments for the bash-builtin printf command (printf "\x34\x12"; printf "\x78\x56"; ...)
>> > - executed generated printf calls as a bash script and redirected stdout to a file
>> > - converted the binary file with uuencode
>> > - concatenated the .uu file with a corresponding license text
>> >
>> > About the sync with FreeBSD's: the truth is I didn't think about that.
>> >
>> > ----------------------------------------
>> > Submit #3044: isp: Unify firmware handling with the rest of the system.
>> > http://bugs.dragonflybsd.org/issues/3044#change-13167
>> >
>> > * Author:
>> > * Status: In Progress
>> > * Priority: Normal
>> > * Assignee: swildner
>> > * Category:
>> > * Target version:
>> > ----------------------------------------
>> > Hello,
>> >
>> > these patches convert firmware binary data from C arrays in a header files to uuencode .uu files. Byte order of the firmware image is little-endian. As far as DragonFly BSD supports only little-endian architectures it's ok. But in case of a big-endian architecture the byte order must be changed before firmware is processed by the isp driver. They also move the .uu images to a sys/contrib/dev/isp.
>> >
>> > The first part concerns these firmwares: isp_1040, isp_1040_it, isp_1080, isp_1080_it, isp_12160, isp_12160_it, isp_2100, isp_2200, isp_2300 and isp_2322.
>> > The second part concerns these firmwares: isp_2400, isp_2400_multi, isp_2500, isp_2500_multi.
>> >
>> > The division is needed because a size of a single patch must be less than 5 MB for the bugtracker to accept.
>> >
>> >
>> > ---Files--------------------------------
>> > 0001-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (3.47 MB)
>> > 0002-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (4.02 MB)
>> >
>> >
>> > --
>> > You have received this notification because you have either subscribed to it, or are involved in it.
>> > To change your notification preferences, please click here: http://bugs.dragonflybsd.org/my/account
>>
>>
>>
>> --
>> Tomorrow Will Never Die
>
>
>
> ----------------------------------------
> Submit #3044: isp: Unify firmware handling with the rest of the system.
> http://bugs.dragonflybsd.org/issues/3044#change-13169
>
> * Author:
> * Status: In Progress
> * Priority: Normal
> * Assignee: swildner
> * Category:
> * Target version:
> ----------------------------------------
> Hello,
>
> these patches convert firmware binary data from C arrays in a header files to uuencode .uu files. Byte order of the firmware image is little-endian. As far as DragonFly BSD supports only little-endian architectures it's ok. But in case of a big-endian architecture the byte order must be changed before firmware is processed by the isp driver. They also move the .uu images to a sys/contrib/dev/isp.
>
> The first part concerns these firmwares: isp_1040, isp_1040_it, isp_1080, isp_1080_it, isp_12160, isp_12160_it, isp_2100, isp_2200, isp_2300 and isp_2322.
> The second part concerns these firmwares: isp_2400, isp_2400_multi, isp_2500, isp_2500_multi.
>
> The division is needed because a size of a single patch must be less than 5 MB for the bugtracker to accept.
>
>
> ---Files--------------------------------
> 0001-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (3.47 MB)
> 0002-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (4.02 MB)
>
>
> --
> You have received this notification because you have either subscribed to it, or are involved in it.
> To change your notification preferences, please click here: http://bugs.dragonflybsd.org/my/account

--
Tomorrow Will Never Die

#8 Updated by sucanjan@fit.cvut.cz about 2 months ago

I have the tool finished.

During the work I discovered a mistake I made. In the asm_2100.h file there are two variants of firmware image for the same device. When I was processing the file I concatenated both variants to one image. Which variant is compiled depends on if USE_SMALLER_2100_FIRMWARE is defined or not.
The solution I would like to implement is to generate two images (one for each variant) and place the conditional selection to the sys/dev/disk/isp_pci.c to the isp_pci_attach() function where the decision which image to get by firmware_get() is made.

Could it be done in this way?

sepherosa wrote:
> Hmm, I wanted to mean, the tool could be isp specific. Take an isp
> firmware header file, and
> convert it to uu file. The tool does not need to be generic; it is
> only used by us to sync other BSD's isp firmware.

#9 Updated by swildner about 2 months ago

I was wondering, is the firmware perhaps more or less easily downloadble from Qlogic and we should take it from them directly.

#10 Updated by sucanjan@fit.cvut.cz about 2 months ago

I have searched the Qlogic web site and haven't got any useful results. It seems that for the older cards (and also for some newer) there are no files for download, and the mapping between device identification and firmwares is not very clear. I think that the "more or less easily downloadable" requirement is not satisfied and it will be easier to use other BSD systems as a source of the firmware.

swildner wrote:
> I was wondering, is the firmware perhaps more or less easily downloadble from Qlogic and we should take it from them directly.

#11 Updated by sucanjan@fit.cvut.cz about 2 months ago

This is the conversion script for the firmware header files for a review.

#12 Updated by sucanjan@fit.cvut.cz about 2 months ago

This is the second version of the patches. I added the conversion script as sys/tools/fw_convert_ispfw.sh, replaced firmware .uu files by those, generated by the script, added smaller variant of the isp_2100 firmware and modified isp_pci.c file for conditional usage of the smaller variant. The condition was originally placed in the sys/dev/disk/ispfw/asm_2100.h file which the patch deletes.

#13 Updated by swildner about 2 months ago

Looks fine, thanks. Also for the script.

I've tested it and the firmware is loaded here with ispfw in the kernel config as well as with isp_1040_load=yes in /boot/loader.conf.

One question though: Should/can we preserve the ispfw.ko module (one module with all firmwares) that we had before? If not, I'll add removing ispfw.ko to 'make upgrade'.

#14 Updated by sucanjan@fit.cvut.cz about 2 months ago

I think it is good to remove ispfw.ko. Looking at the sys/dev/disk/ispfw/Makefile it seems, that ispfw.ko is built alongside of all the other modules which contain individual firmwares so user will end up with two copies of the same firmware images laying on a filesystem (one in the ispfw.ko and one in the individual module).

But manual page should be updated accordingly. I will take some inspiration from iwn(4) manual page.

Should I upload a new set of the patches with the manual page modified, or it will be sufficient to create a third patch for the modification?

#15 Updated by swildner about 2 months ago

A third patch should be fine.

#17 Updated by swildner about 2 months ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100

I've pushed your patches. Thanks!

I've lumped all the patches together into one commit, I hope that's ok. Also, I've changed a few things, such as some different wording in manual pages, adding removal of ispfw.ko to 'make upgrade', moving the script out of sys/tools which is for scripts only that are actually executed during buildkernel, etc.

If you see any issues or disagree with any of my changes, please tell me.

#18 Updated by sucanjan@fit.cvut.cz about 2 months ago

Everything is ok. Thank you very much for the fixes.

At this place I would like to generally remind (for all those who observe these changes) what should be the intended purpose the changes. It should be preparation for firmware subsystem modification so that firmware images can be moved from kernel modules to userland. I think that it is needless to have firmware images wrapped by the kernel module code if they doesn't need to be compiled in the kernel. I believe that FreeBSD could also consider it a good idea and adopt it. I would like to put some ideas for a broader discussion on the IRC or mailing list.

I would like to ask for another thing about the isp driver: should I create a patch to cope with endianness of the isp firmware even if DragonFly currently supports only a little-endian?
These could be some possible approaches: 1) to allocate a buffer for big-endian firmware data which will be created from little-endian version, then firmware_put() the little-endian firmware and replace firmware address by the address of the big-endian version buffer, or 2) to trace every endianness dependent access to the little-endian data and wrap it by sys/endian.h functions.

#19 Updated by sepherosa about 2 months ago

On Fri, Jun 30, 2017 at 4:50 AM,
<> wrote:
> Issue #3044 has been updated by .
>
>
> Everything is ok. Thank you very much for the fixes.
>
> At this place I would like to generally remind (for all those who observe these changes) what should be the intended purpose the changes. It should be preparation for firmware subsystem modification so that firmware images can be moved from kernel modules to userland. I think that it is needless to have firmware images wrapped by the kernel module code if they doesn't need to be compiled in the kernel. I believe that FreeBSD could also consider it a good idea and adopt it. I would like to put some ideas for a broader discussion on the IRC or mailing list.
>
> I would like to ask for another thing about the isp driver: should I create a patch to cope with endianness of the isp firmware even if DragonFly currently supports only a little-endian?
> These could be some possible approaches: 1) to allocate a buffer for big-endian firmware data which will be created from little-endian version, then firmware_put() the little-endian firmware and replace firmware address by the address of the big-endian version buffer, or 2) to trace every endianness dependent access to the little-endian data and wrap it by sys/endian.h functions.
>

I think we can add this as the later steps of your firmware work.
BTW, you can send the idea and discussion to kernel@, so they would be
recorded :)

Thanks,
sephe

>
> ----------------------------------------
> Submit #3044: isp: Unify firmware handling with the rest of the system.
> http://bugs.dragonflybsd.org/issues/3044#change-13181
>
> * Author:
> * Status: Closed
> * Priority: Normal
> * Assignee: swildner
> * Category:
> * Target version:
> ----------------------------------------
> Hello,
>
> these patches convert firmware binary data from C arrays in a header files to uuencode .uu files. Byte order of the firmware image is little-endian. As far as DragonFly BSD supports only little-endian architectures it's ok. But in case of a big-endian architecture the byte order must be changed before firmware is processed by the isp driver. They also move the .uu images to a sys/contrib/dev/isp.
>
> The first part concerns these firmwares: isp_1040, isp_1040_it, isp_1080, isp_1080_it, isp_12160, isp_12160_it, isp_2100, isp_2200, isp_2300 and isp_2322.
> The second part concerns these firmwares: isp_2400, isp_2400_multi, isp_2500, isp_2500_multi.
>
> The division is needed because a size of a single patch must be less than 5 MB for the bugtracker to accept.
>
>
> ---Files--------------------------------
> 0001-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (3.47 MB)
> 0002-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (4.02 MB)
> convert.sh (8.24 KB)
> 0001-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (3.49 MB)
> 0002-isp-Unify-firmware-handling-with-the-rest-of-the-sys.patch (4.02 MB)
> 0003-ispfw.4-Use-modules-with-individual-firmwares-instea.patch (1.46 KB)
> 0004-isp.4-Modules-with-individual-firmwares-can-be-loade.patch (1.46 KB)
>
>
> --
> You have received this notification because you have either subscribed to it, or are involved in it.
> To change your notification preferences, please click here: http://bugs.dragonflybsd.org/my/account

--
Tomorrow Will Never Die

#20 Updated by swildner about 2 months ago

I don't think an big-endian patch is strictly needed, but it's up to you.

Also available in: Atom PDF