2012-02-13 20:38:42

by Divy Le ray

[permalink] [raw]
Subject: [patch net-next] cxgb3: update firmware version


Hi Dave,

This patch is sent in conjunction with the cxgb3 firmware update submission on linux-firmware.
(https://lkml.org/lkml/2012/2/13/27)

Cheers,
Divy

From: Divy Le Ray <[email protected]>

This patch updates the firmware version cxgb3 is requesting to 7.12.0.

Signed-off-by: Divy Le Ray <[email protected]>
---

drivers/net/ethernet/chelsio/cxgb3/version.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/drivers/net/ethernet/chelsio/cxgb3/version.h b/drivers/net/ethernet/chelsio/cxgb3/version.h
index 8bda06e..165bfb9 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/version.h
+++ b/drivers/net/ethernet/chelsio/cxgb3/version.h
@@ -35,10 +35,10 @@
#define DRV_DESC "Chelsio T3 Network Driver"
#define DRV_NAME "cxgb3"
/* Driver version */
-#define DRV_VERSION "1.1.4-ko"
+#define DRV_VERSION "1.1.5-ko"

/* Firmware version */
#define FW_VERSION_MAJOR 7
-#define FW_VERSION_MINOR 10
+#define FW_VERSION_MINOR 12
#define FW_VERSION_MICRO 0
#endif /* __CHELSIO_VERSION_H */


2012-02-13 20:43:47

by David Woodhouse

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

On Mon, 2012-02-13 at 12:28 -0800, Divy Le Ray wrote:
> This patch updates the firmware version cxgb3 is requesting to 7.12.0.

I'm confused. Your patch doesn't change anything in the driver, except
the firmware version. So why is it a "major" change?

If it's a *compatible* change then we wouldn't normally change the
soname of a library; why do we change the filename of the firmware
(which is the equivalent operation)?

--
dwmw2


Attachments:
smime.p7s (5.68 kB)

2012-02-13 21:40:02

by Divy Le ray

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

On Monday, February 13, 2012 12:43:37 PM, David Woodhouse wrote:
>
> On Mon, 2012-02-13 at 12:28 -0800, Divy Le Ray wrote:
>>
>> This patch updates the firmware version cxgb3 is requesting to 7.12.0.
>
>
> I'm confused. Your patch doesn't change anything in the driver, except
> the firmware version. So why is it a "major" change?

Hi David,

cxgb3 embeds the firmware file name it will request at load time.
It uses the FW_VERSION_* defs to construct the firmware name FW_FNAME
before calling request_firmware().
Hence the need to update the firmware minor version.

On the other hand, the driver version update is not a strict requirement
here.
I would prefer keeping it though.

Cheers,
Divy

2012-02-14 01:59:22

by Ben Hutchings

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

On Mon, 2012-02-13 at 13:39 -0800, Divy Le Ray wrote:
> On Monday, February 13, 2012 12:43:37 PM, David Woodhouse wrote:
> >
> > On Mon, 2012-02-13 at 12:28 -0800, Divy Le Ray wrote:
> >>
> >> This patch updates the firmware version cxgb3 is requesting to 7.12.0.
> >
> >
> > I'm confused. Your patch doesn't change anything in the driver, except
> > the firmware version. So why is it a "major" change?
>
> Hi David,
>
> cxgb3 embeds the firmware file name it will request at load time.
> It uses the FW_VERSION_* defs to construct the firmware name FW_FNAME
> before calling request_firmware().
> Hence the need to update the firmware minor version.
>
> On the other hand, the driver version update is not a strict requirement
> here.
> I would prefer keeping it though.

The problems with this are:
1. Older kernel versions don't benefit from the firmware update.
2. The old firmware has to be kept in linux-firmware to support those
old kernel versions, and distributions may have to package more
versions.

To avoid this, the firmware filename should only be changed if you make
incompatible changes in the driver/firmware interface.

Ben.

--
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2012-02-14 04:49:58

by Divy Le ray

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

On Monday, February 13, 2012 5:59:14 PM, Ben Hutchings wrote:
>
> On Mon, 2012-02-13 at 13:39 -0800, Divy Le Ray wrote:
>>
>> On Monday, February 13, 2012 12:43:37 PM, David Woodhouse wrote:
>>>
>>>
>>> On Mon, 2012-02-13 at 12:28 -0800, Divy Le Ray wrote:
>>>>
>>>>
>>>> This patch updates the firmware version cxgb3 is requesting to 7.12.0.
>>>
>>>
>>>
>>> I'm confused. Your patch doesn't change anything in the driver, except
>>> the firmware version. So why is it a "major" change?
>>
>>
>> Hi David,
>>
>> cxgb3 embeds the firmware file name it will request at load time.
>> It uses the FW_VERSION_* defs to construct the firmware name FW_FNAME
>> before calling request_firmware().
>> Hence the need to update the firmware minor version.
>>
>> On the other hand, the driver version update is not a strict requirement
>> here.
>> I would prefer keeping it though.
>
>
> The problems with this are:
> 1. Older kernel versions don't benefit from the firmware update.
> 2. The old firmware has to be kept in linux-firmware to support those
> old kernel versions, and distributions may have to package more
> versions.
>
> To avoid this, the firmware filename should only be changed if you make
> incompatible changes in the driver/firmware interface.

Hi Ben,

The scheme for cxgb3 firmware update was designed in coordination with
OEMs in the early stages of the driver existence back in 2007.
Committing FW 7.12.0 under the name of t3fw-7.10.0.bin would be
confusing, in my mind.
Some distributions have explicitly requested the FW rev number to be
appended to the firmware file name.

Cheers,
Divy

2012-02-14 05:08:42

by Ben Hutchings

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

On Mon, 2012-02-13 at 20:49 -0800, Divy Le Ray wrote:
> On Monday, February 13, 2012 5:59:14 PM, Ben Hutchings wrote:
> >
> > On Mon, 2012-02-13 at 13:39 -0800, Divy Le Ray wrote:
> >>
> >> On Monday, February 13, 2012 12:43:37 PM, David Woodhouse wrote:
> >>>
> >>>
> >>> On Mon, 2012-02-13 at 12:28 -0800, Divy Le Ray wrote:
> >>>>
> >>>>
> >>>> This patch updates the firmware version cxgb3 is requesting to 7.12.0.
> >>>
> >>>
> >>>
> >>> I'm confused. Your patch doesn't change anything in the driver, except
> >>> the firmware version. So why is it a "major" change?
> >>
> >>
> >> Hi David,
> >>
> >> cxgb3 embeds the firmware file name it will request at load time.
> >> It uses the FW_VERSION_* defs to construct the firmware name FW_FNAME
> >> before calling request_firmware().
> >> Hence the need to update the firmware minor version.
> >>
> >> On the other hand, the driver version update is not a strict requirement
> >> here.
> >> I would prefer keeping it though.
> >
> >
> > The problems with this are:
> > 1. Older kernel versions don't benefit from the firmware update.
> > 2. The old firmware has to be kept in linux-firmware to support those
> > old kernel versions, and distributions may have to package more
> > versions.
> >
> > To avoid this, the firmware filename should only be changed if you make
> > incompatible changes in the driver/firmware interface.
>
> Hi Ben,
>
> The scheme for cxgb3 firmware update was designed in coordination with
> OEMs in the early stages of the driver existence back in 2007.

I'm aware that some OEMs have specifications for flash firmware updates
and version numbers. I don't recall seeing anything about naming
firmware files that are loaded by the driver at boot.

In any case, whatever your OEMs want in out-of-tree driver packages is
not necessarily relevant to mainline Linux.

> Committing FW 7.12.0 under the name of t3fw-7.10.0.bin would be
> confusing, in my mind.

Indeed it would, but I was speaking in general terms - that it would be
preferable to use something like t3fw-<major>.bin.

I assume that the firmware blob has a version embedded in it, so that
the driver would still be able to report the exact version through
ethtool_ops::get_drvinfo.

> Some distributions have explicitly requested the FW rev number to be
> appended to the firmware file name.

Did they really want the full version there or did they want you to
distinguish incompatible versions? Did they assume that the file would
be installed by a package (so that the package carries a version number)
or not?

Ben.

--
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2012-02-14 05:56:29

by Divy Le ray

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

On 2/13/2012 9:08 PM, Ben Hutchings wrote:
> On Mon, 2012-02-13 at 20:49 -0800, Divy Le Ray wrote:
>> On Monday, February 13, 2012 5:59:14 PM, Ben Hutchings wrote:
>>> On Mon, 2012-02-13 at 13:39 -0800, Divy Le Ray wrote:
>>>> On Monday, February 13, 2012 12:43:37 PM, David Woodhouse wrote:
>>>>>
>>>>> On Mon, 2012-02-13 at 12:28 -0800, Divy Le Ray wrote:
>>>>>>
>>>>>> This patch updates the firmware version cxgb3 is requesting to 7.12.0.
>>>>>
>>>>>
>>>>> I'm confused. Your patch doesn't change anything in the driver, except
>>>>> the firmware version. So why is it a "major" change?
>>>>
>>>> Hi David,
>>>>
>>>> cxgb3 embeds the firmware file name it will request at load time.
>>>> It uses the FW_VERSION_* defs to construct the firmware name FW_FNAME
>>>> before calling request_firmware().
>>>> Hence the need to update the firmware minor version.
>>>>
>>>> On the other hand, the driver version update is not a strict requirement
>>>> here.
>>>> I would prefer keeping it though.
>>>
>>> The problems with this are:
>>> 1. Older kernel versions don't benefit from the firmware update.
>>> 2. The old firmware has to be kept in linux-firmware to support those
>>> old kernel versions, and distributions may have to package more
>>> versions.
>>>
>>> To avoid this, the firmware filename should only be changed if you make
>>> incompatible changes in the driver/firmware interface.
>> Hi Ben,
>>
>> The scheme for cxgb3 firmware update was designed in coordination with
>> OEMs in the early stages of the driver existence back in 2007.
> I'm aware that some OEMs have specifications for flash firmware updates
> and version numbers. I don't recall seeing anything about naming
> firmware files that are loaded by the driver at boot.
>
> In any case, whatever your OEMs want in out-of-tree driver packages is
> not necessarily relevant to mainline Linux.

Ben, it was about the driver committed to mainline linux.

>> Committing FW 7.12.0 under the name of t3fw-7.10.0.bin would be
>> confusing, in my mind.
> Indeed it would, but I was speaking in general terms - that it would be
> preferable to use something like t3fw-<major>.bin.

Okay, I understand now. We'd have to discuss it with the original OEMS
and distributions.
I'd rather stick to the current scheme for the time being and update it
once we have agreement from all parties.

> I assume that the firmware blob has a version embedded in it, so that
> the driver would still be able to report the exact version through
> ethtool_ops::get_drvinfo.
>
>> Some distributions have explicitly requested the FW rev number to be
>> appended to the firmware file name.
> Did they really want the full version there or did they want you to
> distinguish incompatible versions? Did they assume that the file would
> be installed by a package (so that the package carries a version number)
> or not?

Our orginal approach for cxgb4 was to commit its firmware under a
constant name.
Following minor FW updates overwrite the existing firmware file, and it
raised some negative feedback.
The cxgb3 scheme had not raised any concern until today, it seemed
natural to get back to
a similar approach for cxgb4, at least partially.

Cheers,
Divy

2012-02-14 18:17:33

by Rick Jones

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

What does an end-user/customer see and do? When they are told that
version the firmware, specified down to the fix level, fixes a problem
they are having, how will they know that firmware blob filename
drvfw-X.bin on a download/archive site has that fix? Yes, the support
folks can say "It is the drcfw-X.bin file which is 12345 bytes in size
with the MD5sum of <blah> but the principle of the telephone game
dictates the size and/or MD5sum information will drop as "the word"
passes from one person to another. And one cannot run
ethtool_ops::get_drvinfo against a website. Two versions may indeed be
"compatible" in terms of interfaces, but they won't be identical in
terms of desirability. As such, for them to be contained in files with
identical names just seems like asking for confusion.

Firmware is something of a tiny kernel no? Would we have everyone name
vmlinux files with just a major number?

rick jones

2012-02-14 18:21:25

by Ben Hutchings

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

On Tue, Feb 14, 2012 at 10:17:27AM -0800, Rick Jones wrote:
> What does an end-user/customer see and do? When they are told that
> version the firmware, specified down to the fix level, fixes a
> problem they are having, how will they know that firmware blob
> filename drvfw-X.bin on a download/archive site has that fix?
[...]

Customers currently can't get bug fixes by updating the firmware.
Think about it.

Ben.

--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus

2012-02-14 20:02:32

by Rick Jones

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

On 02/14/2012 10:21 AM, Ben Hutchings wrote:
> On Tue, Feb 14, 2012 at 10:17:27AM -0800, Rick Jones wrote:
>> What does an end-user/customer see and do? When they are told that
>> version the firmware, specified down to the fix level, fixes a
>> problem they are having, how will they know that firmware blob
>> filename drvfw-X.bin on a download/archive site has that fix?
> [...]
>
> Customers currently can't get bug fixes by updating the firmware.
> Think about it.

Because in this case at least the driver is requesting a specific
filename down to the minor and micro level and a change in the filename
requires a change in the driver?

#define FW_VERSION __stringify(FW_VERSION_MAJOR) "." \
__stringify(FW_VERSION_MINOR) "." __stringify(FW_VERSION_MICRO)
#define FW_FNAME "cxgb3/t3fw-" FW_VERSION ".bin"

rather than the driver having logic to make it willing to accept later
minor/micro changes by being more sophisticated in its specification of
firmware file.

2012-02-14 22:03:03

by Ben Hutchings

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

On Tue, Feb 14, 2012 at 12:02:26PM -0800, Rick Jones wrote:
> On 02/14/2012 10:21 AM, Ben Hutchings wrote:
> >On Tue, Feb 14, 2012 at 10:17:27AM -0800, Rick Jones wrote:
> >>What does an end-user/customer see and do? When they are told that
> >>version the firmware, specified down to the fix level, fixes a
> >>problem they are having, how will they know that firmware blob
> >>filename drvfw-X.bin on a download/archive site has that fix?
> >[...]
> >
> >Customers currently can't get bug fixes by updating the firmware.
> >Think about it.
>
> Because in this case at least the driver is requesting a specific
> filename down to the minor and micro level and a change in the
> filename requires a change in the driver?
>
> #define FW_VERSION __stringify(FW_VERSION_MAJOR) "." \
> __stringify(FW_VERSION_MINOR) "." __stringify(FW_VERSION_MICRO)
> #define FW_FNAME "cxgb3/t3fw-" FW_VERSION ".bin"
>
> rather than the driver having logic to make it willing to accept
> later minor/micro changes by being more sophisticated in its
> specification of firmware file.

Well, you can't really be very sophisticated as there is no way to
list the available firmware files.

The Intel wireless drivers try iterating from the newest to oldest
supported API version. But that API version is changed only rarely.
If you were to iterate over some wide range of potential future
versions it could take a substantial time to find the file that's
actualy installed.

Ben.

--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus

2012-02-15 20:51:18

by David Miller

[permalink] [raw]
Subject: Re: [patch net-next] cxgb3: update firmware version

From: Divy Le Ray <[email protected]>
Date: Mon, 13 Feb 2012 12:28:46 -0800

> This patch updates the firmware version cxgb3 is requesting to 7.12.0.
>
> Signed-off-by: Divy Le Ray <[email protected]>

While I agree with Ben Hutchings that how this is being handled is terrible,
it's the current status-quo so I'll apply this.