Patchwork [net-next] cxgb3: update firmware version

login
register
mail settings
Submitter Divy Le Ray
Date Feb. 13, 2012, 8:28 p.m.
Message ID <20120213202846.631.30806.stgit@speedy5.asicdesigners.com>
Download mbox | patch
Permalink /patch/140984/
State Accepted
Delegated to: David Miller
Headers show

Comments

Divy Le Ray - Feb. 13, 2012, 8:28 p.m.
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 <divy@chelsio.com>

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

Signed-off-by: Divy Le Ray <divy@chelsio.com>
---

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



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Woodhouse - Feb. 13, 2012, 8:43 p.m.
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)?
Divy Le Ray - Feb. 13, 2012, 9:39 p.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Feb. 14, 2012, 1:59 a.m.
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.
Divy Le Ray - Feb. 14, 2012, 4:49 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Feb. 14, 2012, 5:08 a.m.
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.
Divy Le Ray - Feb. 14, 2012, 5:55 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rick Jones - Feb. 14, 2012, 6:17 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Feb. 14, 2012, 6:21 p.m.
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.
Rick Jones - Feb. 14, 2012, 8:02 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Feb. 14, 2012, 10:02 p.m.
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.
David Miller - Feb. 15, 2012, 8:48 p.m.
From: Divy Le Ray <divy@chelsio.com>
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 <divy@chelsio.com>

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 */