On Thu, Dec 12, 2019 at 01:40:27AM +0000, Saleem, Shiraz wrote: > > Subject: Re: [PATCH v3 05/20] RDMA/irdma: Add driver framework definitions <...> > > > > > + ldev->ops->reg_for_notification(ldev, &events); > > > + dev_info(rfdev_to_dev(dev), "IRDMA VSI Open Successful"); > > > > Lets not do this kind of logging.. > > > > There is some dev_info which should be cleaned up to dev_dbg. > But logging this info is useful to know that this functions VSI (and associated ibdev) > is up and reading for RDMA traffic. > Is info logging to be avoided altogether? Will function tracer (ftrace) output be sufficient here? https://www.kernel.org/doc/html/latest/trace/ftrace.html > > > > +static void irdma_close(struct iidc_peer_dev *ldev, enum > > > +iidc_close_reason reason) { > > > + struct irdma_device *iwdev; > > > + struct irdma_pci_f *rf; > > > + > > > + iwdev = irdma_get_device(ldev->netdev); > > > + if (!iwdev) > > > + return; > > > + > > > + irdma_put_device(iwdev); > > > + rf = iwdev->rf; > > > + if (reason == IIDC_REASON_GLOBR_REQ || reason == > > IIDC_REASON_CORER_REQ || > > > + reason == IIDC_REASON_PFR_REQ || rf->reset) { > > > + iwdev->reset = true; > > > + rf->reset = true; > > > + } > > > + > > > + if (iwdev->init_state >= CEQ0_CREATED) > > > + irdma_deinit_rt_device(iwdev); > > > + > > > + kfree(iwdev); > > > > Mixing put and kfree? So confusing. Why are there so many structs and so much > > indirection? Very hard to understand if this is right or not. > > This does look weird. I think the irdma_get_device() was here > just to get to iwdev. And put_device is releasing the refcnt immediately. > Since we are in a VSI close(), we should not need to take refcnt on ibdev > and just deregister it. Will fix this. > > > > > > new file mode 100644 > > > index 000000000000..b418e76a3302 > > > +++ b/drivers/infiniband/hw/irdma/main.c > > > @@ -0,0 +1,630 @@ > > > +// SPDX-License-Identifier: GPL-2.0 or Linux-OpenIB > > > +/* Copyright (c) 2015 - 2019 Intel Corporation */ #include "main.h" > > > + > > > +/* Legacy i40iw module parameters */ > > > +static int resource_profile; > > > +module_param(resource_profile, int, 0644); > > > +MODULE_PARM_DESC(resource_profile, "Resource Profile: 0=PF only, > > > +1=Weighted VF, 2=Even Distribution"); > > > + > > > +static int max_rdma_vfs = 32; > > > +module_param(max_rdma_vfs, int, 0644); > > MODULE_PARM_DESC(max_rdma_vfs, > > > +"Maximum VF count: 0-32 32=default"); > > > + > > > +static int mpa_version = 2; > > > +module_param(mpa_version, int, 0644); MODULE_PARM_DESC(mpa_version, > > > +"MPA version: deprecated parameter"); > > > + > > > +static int push_mode; > > > +module_param(push_mode, int, 0644); > > > +MODULE_PARM_DESC(push_mode, "Low latency mode: deprecated > > > +parameter"); > > > + > > > +static int debug; > > > +module_param(debug, int, 0644); > > > +MODULE_PARM_DESC(debug, "debug flags: deprecated parameter"); > > > > Generally no to module parameters > > Agree. But these are module params that existed in i40iw. > And irdma replaces i40iw and has a module alias > for it. Maybe use this opportunity and ditch "deprecated" module parameters? Thanks
