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

[Michael Reed]

Hi Tim,

Thanks for the feedback.  I am a  victim of over cutting and pasting from
the private bug for comment #2.  That is feedback from Cisco.

On Sun, Aug 14, 2022 at 9:44 AM Tim Gardner <tim.gardner at canonical.com>
wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20220815/c891afcf/attachment.html>