On Saturday, June 15, 2024 2:05 AM, Hannes Reinecke <hare@xxxxxxx> wrote: > > On 6/15/24 05:44, Karan Tilak Kumar (kartilak) wrote: > > On Tuesday, June 11, 2024 11:48 PM, Hannes Reinecke <hare@xxxxxxx> 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. Thanks for your comments, Hannes. I understand your concern. In this patch, I've replaced some of the existing code to use FDLS to avoid code duplication. I've tested the whole patch series with all the FDLS changes on our blade and rack servers for several weeks to _not_ impact functionality. (Please refer to the cover letter to note the set of tests performed to test FDLS). Therefore, if the entire patch series were to be applied, based on my tests, fnic driver based on FDLS should work seamlessly. Please let me know if you have any questions regarding this. > 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. > Thanks for your encouraging words, Hannes. Based on my previous comments, we (the Cisco team) feel it is easier to maintain a single driver. We will be mindful of your comments while making changes for the next version of changes. Regards, Karan
