This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 504181 - [Broadcom 5.4 bug] Include fixes/cleanups for bnx2i [NEEDINFO]
[Broadcom 5.4 bug] Include fixes/cleanups for bnx2i
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: kernel (Show other bugs)
5.4
All Linux
high Severity high
: rc
: 5.4
Assigned To: Mike Christie
Red Hat Kernel QE team
: OtherQA
Depends On: 441979
Blocks: 458757 511096
  Show dependency treegraph
 
Reported: 2009-06-04 12:04 EDT by Mike Christie
Modified: 2009-09-03 09:55 EDT (History)
14 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-09-02 04:31:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
cward: needinfo? (gregorym)


Attachments (Terms of Use)
cnic cleanup patch (1/4) (3.25 KB, patch)
2009-06-09 17:18 EDT, Michael Chan
no flags Details | Diff
uio BKL removal patch (2/4) (1.53 KB, patch)
2009-06-09 17:20 EDT, Michael Chan
no flags Details | Diff
bnx2i/cnic cleanup patch 3/4 (2.02 KB, patch)
2009-06-09 17:22 EDT, Michael Chan
no flags Details | Diff
bnx2i bug fix and cleanup (4/4) (10.24 KB, patch)
2009-06-09 17:28 EDT, Michael Chan
no flags Details | Diff
New bnx2i cleanup patch (4/4) (12.04 KB, patch)
2009-06-12 17:10 EDT, Michael Chan
no flags Details | Diff
Newest bnx2i cleanup patch (4/4) (18.66 KB, patch)
2009-06-24 01:45 EDT, Michael Chan
no flags Details | Diff
Fix ep_connect() if shost is specified (2.43 KB, patch)
2009-07-08 22:27 EDT, Michael Chan
no flags Details | Diff

  None (edit)
Description Mike Christie 2009-06-04 12:04:56 EDT
Description of problem:

RHEL reviewers found some issues with the bnx2i driver including
- rhel patch had a ifdef LINUX_VERSION that is of course not needed
- rhel patch had implemented a callout that is not called and not upstream so it should be removed
- rhel patch had some bnx2i rhel compat stuff that was not needed because in another patch I added it to a common iscsi compat.h file. 

Optional
- Remove BKL use if possible


While reviewing their review comments and testing new patches I found the following issues:
- wait_event_interruptible is called with spin locks held in a couple places.


Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:
Comment 1 RHEL Product and Program Management 2009-06-04 12:21:10 EDT
This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux maintenance release.  Product Management has requested
further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed
products.  This request is not yet committed for inclusion in an Update
release.
Comment 2 Michael Chan 2009-06-09 14:13:44 EDT
There is also a fix in the handling of the expected statsn in the upstream bnx2i version.  Please include it.
Comment 3 Mike Christie 2009-06-09 14:53:16 EDT
Could you send me a patch for all the bnx2i changes you need/want. I thought you attached that to 441979.
Comment 4 Michael Chan 2009-06-09 15:05:06 EDT
I was attaching the cleanup/lock fix/bug fix patches to 441979 to bring everything inline with upstream this morning, but Andrius stopped me.  Should I attach those patches here?  The patches will address the problems in the Description plus the fix for expected statsn.
Comment 5 Mike Christie 2009-06-09 15:16:24 EDT
Yeah, please do.

Sorry for the confusion. I thought you had uploaded everything before Andrius busted you :)
Comment 6 Michael Chan 2009-06-09 17:18:57 EDT
Created attachment 347090 [details]
cnic cleanup patch (1/4)

Patch for -152.el5 kernel to remove LINUX_VERSION_CODE and to change nl_msg_recv() to return int.
Comment 7 Michael Chan 2009-06-09 17:20:24 EDT
Created attachment 347091 [details]
uio BKL removal patch (2/4)

Backported uio BKL removal patch for -152.el5 kernel
Comment 8 Michael Chan 2009-06-09 17:22:59 EDT
Created attachment 347092 [details]
bnx2i/cnic cleanup patch 3/4

Remove unused cnic/bnx2i inetevent handler for -152.el5.
Comment 9 Michael Chan 2009-06-09 17:28:09 EDT
Created attachment 347093 [details]
bnx2i bug fix and cleanup (4/4)

bnx2i bug fixes and cleanup for -152.el5:

- Locking bugfix in ep_connect()/ep_disconnect() that was noticed by Mike.
- expected statsn handling fix.
- other minor cleanups.

A big thanks to Mike Christie for helping us get this merged upstream and into RHEL5.4.
Comment 10 Mike Christie 2009-06-09 22:36:47 EDT
Hey Michael,

What is up with the hba->lock around some bnx2i_reg_device accesses? It seems useless since different hbas could have their own lock and increment/decrement it?

Was supposed to be a per hba variable? If not maybe you want to make it atomic?

Also bnx2i_init_one always returns 0. Was it supposed to return rc in the failure path? The only caller checks for a non zero return value.
Comment 11 Michael Chan 2009-06-10 00:05:02 EDT
Yeah, we've actually discussed this when Anil sent out his patch for internal review, but all of us failed to realize that bnx2i_reg_device is global.  I think it is meant to be a global counter but it is not very clear to me the purpose of this counter.  It is only checked in bnx2i_init_one().  It should either be protected by the bnx2i_dev_lock or converted to atomic.

Yeah, bnx2i_init_one() looks buggy to me in the error path.

Adding Anil to CC.
Comment 12 Anil Veerabhadrappa 2009-06-10 10:42:55 EDT
Yeah, 'bnx2i_reg_device' should be defined as atomic. bnx2i assumes register_device() will fail only in case of duplicate registration, but there could be other cases like failed request_irq() and other cases where bnx2i_init_one() should return error code. I update this and send a patch.
Comment 13 Michael Chan 2009-06-12 17:10:45 EDT
Created attachment 347665 [details]
New bnx2i cleanup patch (4/4)

New bnx2i cleanup patch to replace the older one.  Mike found problems with the bnx2i_init_one() error handling path the locking around bnx2i_reg_device.
Comment 15 Michael Chan 2009-06-24 01:45:25 EDT
Created attachment 349194 [details]
Newest bnx2i cleanup patch (4/4)

Newest bnx2i bug fix and cleanup patch to match upstream.
Comment 19 Michael Chan 2009-07-08 22:27:32 EDT
Created attachment 351009 [details]
Fix ep_connect() if shost is specified
Comment 20 Andrius Benokraitis 2009-07-08 22:39:45 EDT
mchan - the patch in Comment #19 will need to be included in a new BZ since mchristi already sent up a patchset (hence it is in POST) for review.
Comment 21 Mike Christie 2009-07-13 12:14:38 EDT
I made a new bugzilla for this and a bug in libiscsi I found while testing it. The bz is here https://bugzilla.redhat.com/show_bug.cgi?id=511096
Comment 22 Don Zickus 2009-07-14 16:57:19 EDT
in kernel-2.6.18-158.el5
You can download this test kernel from http://people.redhat.com/dzickus/el5

Please do NOT transition this bugzilla state to VERIFIED until our QE team
has sent specific instructions indicating when to do so.  However feel free
to provide a comment indicating that this fix has been verified.
Comment 24 Caspar Zhang 2009-07-29 01:19:22 EDT
Verified that the patch is included in kernel-2.6.18-160.el5
Comment 25 Chris Ward 2009-08-03 11:47:03 EDT
~~ Attention Partners - RHEL 5.4 Snapshot 5 Released! ~~

RHEL 5.4 Snapshot 5 is the FINAL snapshot to be release before RC. It has been 
released on partners.redhat.com. If you have already reported your test results, 
you can safely ignore this request. Otherwise, please notice that there should be 
a fix available now that addresses this particular issue. Please test and report 
back your results here, at your earliest convenience.

If you encounter any issues while testing Beta, please describe the 
issues you have encountered and set the bug into NEED_INFO. If you 
encounter new issues, please clone this bug to open a new issue and 
request it be reviewed for inclusion in RHEL 5.4 or a later update, if it 
is not of urgent severity. If it is urgent, escalate the issue to your partner manager as soon as possible. There is /very/ little time left to get additional code into 5.4 before GA.

Partners, after you have verified, do not flip the bug status to VERIFIED. Instead, please set your Partner ID in the Verified field above if you have successfully verified the resolution of this issue. 

Further questions can be directed to your Red Hat Partner Manager or other 
appropriate customer representative.
Comment 26 Andrius Benokraitis 2009-08-04 15:19:43 EDT
Can Broadcom please test this functionality ASAP?
Comment 27 Greg Morrison 2009-08-05 17:41:57 EDT
Testing it right now.  I'll complete testing as fast as possible.
Comment 28 Chris Ward 2009-08-10 05:37:13 EDT
Thanks Greg. Your testing feedback is greatly appreciated.
Comment 30 Chris Ward 2009-09-01 09:21:20 EDT
Greg, and updates?
Comment 31 errata-xmlrpc 2009-09-02 04:31:19 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2009-1243.html

Note You need to log in before you can comment on or make changes to this bug.