ACK/Cmnt: [SRU][F][PATCH 0/5] fnic driver on needs to be updated to 1.6.0.53 on Focal

[Tim Gardner]

On 8/12/22 18:10, Michael Reed wrote:
> From: Michael Reed <Michael.Reed at canonical.com>
> 
> BugLink:  https://launchpad.net/bugs/1984011
> 
> SRU Justification:
> 
> [Impact]
> 
> fNIC driver controls print messages based on the flag fnic_log_level. shost_printk is not controlled via this flag. This issue is resolved by using some of the print macros that have been defined in fnic. This has negligible impact.
> The resid was being set irrespective of whether we saw an underflow or not. It needs to be set only when we see an underflow. The impact here is negligible.
> When we don't receive link events, we could go into a loop before sending fw reset. The patch for the issue resolves this, and we break out of the loop.
> Prior to checking the remote port, fnic driver must check if the io_req is valid. If not, there could be a crash. The patch resolves this issue.
> 
> [Fix]
> The following patches need to be pulled from upstream to update the fnic driver to 1.6.0.53:
> 
> https://marc.info/?l=linux-scsi&m=160591061813369&w=2
> https://marc.info/?l=linux-scsi&m=160592183315837&w=2
> https://marc.info/?l=linux-scsi&m=160592283315997&w=2
> https://marc.info/?l=linux-scsi&m=160592363016122&w=2
> https://marc.info/?l=linux-scsi&m=160592616516616&w=2
> 
> Here's a description of each issue:
> 
> 1. https://marc.info/?l=linux-scsi&m=160591061813369&w=2
> 
> FNIC_FCS_DBG print is controlled by fnic_log_level flag. Replace shost_printk in fnic_fip_handler_timer with this print so that it can be controlled.
> 
> 2. https://marc.info/?l=linux-scsi&m=160592183315837&w=2
> 
> When fnic is in TRANS ETH state, and when there are no link events, we must not loop before sending fw reset.
> 
> 3. https://marc.info/?l=linux-scsi&m=160592283315997&w=2
> 
> FNIC_MAIN_DBG print is controlled by fnic_log_level flag. Replace shost_printk in fnic_handle_link with this print so that it can be controlled.
> 
> 4. https://marc.info/?l=linux-scsi&m=160592363016122&w=2
> 
> Fix to set scsi_set_resid() only if FCPIO_ICMND_CMPL_RESID_UNDER is set.
> 
> 5. https://marc.info/?l=linux-scsi&m=160592616516616&w=2
> 
> Check for a valid io_req before we check other data.
> 
> [Test Plan]
> 
> Runnings IOs with multiple link flaps would be a good test case to validate all the patches above. It is suggested to run these IOs with a data integrity check. We do this as a standard practice.
> 
> [Where problems could occur]
> 
> The print messages are innocuous. It is not expected to run into any issues.
> Problems could occur with storage arrays that have a non-standard response.
> If the sanity test fails, there could be issues in the scsi midlayer or fnic driver.
> 
> All the patches present low regression risk.
> 
> [Other Info]
> https://code.launchpad.net/~mreed8855/ubuntu/+source/linux/+git/focal/+ref/fnic_cisco_ver3
> 
> 
> Karan Tilak Kumar (5):
>    scsi: fnic: Change shost_printk() to FNIC_FCS_DBG()
>    scsi: fnic: Avoid looping in TRANS ETH on unload
>    scsi: fnic: Change shost_printk() to FNIC_MAIN_DBG()
>    scsi: fnic: Set scsi_set_resid() only for underflow
>    scsi: fnic: Validate io_req before others
> 
>   drivers/scsi/fnic/fnic.h      |  3 ++-
>   drivers/scsi/fnic/fnic_fcs.c  | 10 ++++++----
>   drivers/scsi/fnic/fnic_main.c |  2 ++
>   drivers/scsi/fnic/fnic_scsi.c | 17 +++++++++--------
>   4 files changed, 19 insertions(+), 13 deletions(-)
> 
Acked-by: Tim Gardner <tim.gardner at canonical.com>

Test results look good (I think). The comment in 
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1984011/comments/2 
is a bit weird, but I assume its in response to you providing a test 
kernel ? Otherwise, responding to yourself in the third person is just 
schizophrenic.

-- 
-----------
Tim Gardner
Canonical, Inc