Date: Sat, 15 Jun 2024 11:05:00 +0200
From: Hannes Reinecke <hare@...e.de>
To: "Karan Tilak Kumar (kartilak)" <kartilak@...co.com>,
 "Sesidhar Baddela (sebaddel)" <sebaddel@...co.com>
Cc: "Arulprabhu Ponnusamy (arulponn)" <arulponn@...co.com>,
 "Dhanraj Jhawar (djhawar)" <djhawar@...co.com>,
 "Gian Carlo Boffa (gcboffa)" <gcboffa@...co.com>,
 "Masa Kai (mkai2)" <mkai2@...co.com>,
 "Satish Kharat (satishkh)" <satishkh@...co.com>,
 "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
 "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
 "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 07/14] scsi: fnic: Add and integrate support for FIP

On 6/15/24 05:44, Karan Tilak Kumar (kartilak) wrote:
> On Tuesday, June 11, 2024 11:48 PM, Hannes Reinecke <hare@...e.de> wrote:
>>
>> On 6/10/24 23:50, Karan Tilak Kumar wrote:
>>> Add and integrate support for FCoE Initialization
>>> (protocol) FIP. This protocol will be exercised on Cisco UCS rack
>>> servers.
>>> Add support to specifically print FIP related debug messages.
>>> Replace existing definitions to handle new data structures.
>>> Clean up old and obsolete definitions.
>>>
>> Aren't you getting a bit overboard here?
>>
>> I am _positive_ that the original fnic driver _did_ do FIP.
>> What happened to that?
>> Why can't you just use that implementation?
>>
>> And if you can't use that implementation, shouldn't this rather be a new driver instead of replacing
>> most if not all functionality of the original fnic driver?
> 
> Thanks for your review comments, Hannes.
> As you can see from this patch, and some of the later patches, fnic driver would be reliant on FDLS.
> FDLS helps solve some of the issues that we have seen in our hardware where:
> 
> 1) the adapter hangs such that FC offload is impacted,
> 2) the fabric gets into a blackhole situation, where nothing comes out of the fabric.
> 
> These situations get easily escalated and are quite hard to diagnose.
> FDLS has helped us in these instances to chart a way forward, and to solve the issue.
> 
> Cisco has been shipping async fnic driver based on FDLS for the past six years.
> Cisco officially supports the async driver.
> The async driver has support for PC-RSCN (seen in a later patch) and NVME initiators.
> Since the async driver has been in the field for quite a while now, it is a well-seasoned driver.
> It has also gone through lots of QA cycles to reach a level of stability today.
> Therefore, the Cisco team feels comfortable in upstreaming this change.
> 
> Keeping in line with the upstreaming best practices, our preferred line of approach is to modify
> the existing driver with this change.
> I assume that there will be challenges in maintaining two separate upstream drivers and hence the
> current approach.
> However, we want to be mindful of your comments.
> If you believe that a new driver is warranted based on these changes, the Cisco team can evaluate
> that approach.
> Please share your thoughts with us.
> 
In generally, adding new functionality to an existing driver is the 
correct way to do. What I am worrying about is that we avoid code 
duplication (hence my comment for FIP handling), and that existing
functionality is not impacted.
But when adding new functionality one always has to check how much
shared functionality there is; if there is very little overlap it
would make sense to split the driver in two.
However, that is personal preference; if you think that the driver is
easier to maintain as a single driver, that's fine, too.
But your effort in upstreaming the driver is very much appreciated.
Keep up the good work.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@...e.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Powered by blists - more mailing lists