| Message ID | 20150807133610.12421.73468.stgit@phlsvslse11.ph.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, 7 Aug 2015, Mike Marciniszyn wrote: > { > @@ -599,25 +581,21 @@ static ssize_t show_tempsense(struct device *device, > /* start of per-unit file structures and support code */ > static DEVICE_ATTR(hw_rev, S_IRUGO, show_rev, NULL); > static DEVICE_ATTR(board_id, S_IRUGO, show_hfi, NULL); > -static DEVICE_ATTR(version, S_IRUGO, show_version, NULL); > static DEVICE_ATTR(nctxts, S_IRUGO, show_nctxts, NULL); > static DEVICE_ATTR(nfreectxts, S_IRUGO, show_nfreectxts, NULL); > static DEVICE_ATTR(serial, S_IRUGO, show_serial, NULL); > static DEVICE_ATTR(boardversion, S_IRUGO, show_boardversion, NULL); > static DEVICE_ATTR(tempsense, S_IRUGO, show_tempsense, NULL); > -static DEVICE_ATTR(localbus_info, S_IRUGO, show_localbus_info, NULL); > static DEVICE_ATTR(chip_reset, S_IWUSR, NULL, store_chip_reset); AFAICT the remaining are also provided by generic APIs (aside from nctxt, nfreectxts). I really want our management appss for device etc not to crap out. Could you get some experienced engineers to look at the driver internally to Intel before publishing? There are numerous other drivers in the kernel by Intel that do the right thing. That this is duplicated and the other things show issues with kernel basics. The driver in its entirety probably does not follow the quality that we are used to from other developers at Intel. Its likely that structural changes need to be made to the driver. Significant portions may be duplicating functionality that the kernel already provides as generic functionality. Also we have already established that there is significantly duplication of other drivers in the infiniband tree. Should this not go into staging instead? If this is merged then the push to clean these issues up goes away like it did with the earlier incarnations of this hardware. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > { > > @@ -599,25 +581,21 @@ static ssize_t show_tempsense(struct device > > *device, > > /* start of per-unit file structures and support code */ static > > DEVICE_ATTR(hw_rev, S_IRUGO, show_rev, NULL); static > > DEVICE_ATTR(board_id, S_IRUGO, show_hfi, NULL); -static > > DEVICE_ATTR(version, S_IRUGO, show_version, NULL); static > > DEVICE_ATTR(nctxts, S_IRUGO, show_nctxts, NULL); static > > DEVICE_ATTR(nfreectxts, S_IRUGO, show_nfreectxts, NULL); static > > DEVICE_ATTR(serial, S_IRUGO, show_serial, NULL); static > > DEVICE_ATTR(boardversion, S_IRUGO, show_boardversion, NULL); static > > DEVICE_ATTR(tempsense, S_IRUGO, show_tempsense, NULL); -static > > DEVICE_ATTR(localbus_info, S_IRUGO, show_localbus_info, NULL); static > > DEVICE_ATTR(chip_reset, S_IWUSR, NULL, store_chip_reset); > > AFAICT the remaining are also provided by generic APIs (aside from nctxt, > nfreectxts). I really want our management appss for device etc not to crap > out. > All of these remaining files are provided by hardware specific CSRs. There is no Vital Product Data in the hardware as you provided in the prior Mellanox example. I will see if I can release an lspci -vv. What specific management apps would be broke? Do those apps work with qib? At any rate I was a bit presumptive on removing the TODO. I have removed that and I have also updated sysfs.txt to reflect this patch's end state in a v2. > Could you get some experienced engineers to look at the driver internally to > Intel before publishing? There are numerous other drivers in the kernel by > Intel that do the right thing. > I will try to get a review from internal engineers. > That this is duplicated and the other things show issues with kernel basics. > The driver in its entirety probably does not follow the quality that we are > used to from other developers at Intel. Its likely that structural changes need > to be made to the driver. Significant portions may be duplicating > functionality that the kernel already provides as generic functionality. Also > we have already established that there is significantly duplication of other > drivers in the infiniband tree. > We already know about the transport duplication and we are starting to scope that effort. > Should this not go into staging instead? If this is merged then the push to > clean these issues up goes away like it did with the earlier incarnations of > this hardware. The driver IS in staging. The TODO should reflect the rework. Mike -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 7, 2015 at 7:43 PM, Marciniszyn, Mike <mike.marciniszyn@intel.com> wrote: >> Could you get some experienced engineers to look at the driver internally to >> Intel before publishing? There are numerous other drivers in the kernel by >> Intel that do the right thing. > I will try to get a review from internal engineers. In the latest posted you made a note on 125 changes done as of internal review but didn't spare a word on the nature/content of these changes. There's no way to realize from your V4 cover letter if these changes made things better and in what direction. I checked and none of the 24 authors of the OPA driver is among the 222 over-git-history committers of all drivers under drivers/net/ethernet/intel -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/hfi1/TODO b/drivers/staging/hfi1/TODO index 2529149..dc2f56a 100644 --- a/drivers/staging/hfi1/TODO +++ b/drivers/staging/hfi1/TODO @@ -1,6 +1,5 @@ July, 2015 -- Remove unneeded file entries in sysfs - Remove software processing of IB protocol and place in library for use by qib, ipath (if still present), hfi1, and eventually soft-roce - Clean up comments in code around CNP opcode diff --git a/drivers/staging/hfi1/sysfs.c b/drivers/staging/hfi1/sysfs.c index b10e857..b78c728 100644 --- a/drivers/staging/hfi1/sysfs.c +++ b/drivers/staging/hfi1/sysfs.c @@ -466,13 +466,6 @@ static ssize_t show_hfi(struct device *device, struct device_attribute *attr, return ret; } -static ssize_t show_version(struct device *device, - struct device_attribute *attr, char *buf) -{ - /* The string printed here is already newline-terminated. */ - return scnprintf(buf, PAGE_SIZE, "%s", (char *)ib_hfi1_version); -} - static ssize_t show_boardversion(struct device *device, struct device_attribute *attr, char *buf) { @@ -485,17 +478,6 @@ static ssize_t show_boardversion(struct device *device, } -static ssize_t show_localbus_info(struct device *device, - struct device_attribute *attr, char *buf) -{ - struct hfi1_ibdev *dev = - container_of(device, struct hfi1_ibdev, ibdev.dev); - struct hfi1_devdata *dd = dd_from_dev(dev); - - return scnprintf(buf, PAGE_SIZE, "%s\n", dd->lbus_info); -} - - static ssize_t show_nctxts(struct device *device, struct device_attribute *attr, char *buf) { @@ -599,25 +581,21 @@ static ssize_t show_tempsense(struct device *device, /* start of per-unit file structures and support code */ static DEVICE_ATTR(hw_rev, S_IRUGO, show_rev, NULL); static DEVICE_ATTR(board_id, S_IRUGO, show_hfi, NULL); -static DEVICE_ATTR(version, S_IRUGO, show_version, NULL); static DEVICE_ATTR(nctxts, S_IRUGO, show_nctxts, NULL); static DEVICE_ATTR(nfreectxts, S_IRUGO, show_nfreectxts, NULL); static DEVICE_ATTR(serial, S_IRUGO, show_serial, NULL); static DEVICE_ATTR(boardversion, S_IRUGO, show_boardversion, NULL); static DEVICE_ATTR(tempsense, S_IRUGO, show_tempsense, NULL); -static DEVICE_ATTR(localbus_info, S_IRUGO, show_localbus_info, NULL); static DEVICE_ATTR(chip_reset, S_IWUSR, NULL, store_chip_reset); static struct device_attribute *hfi1_attributes[] = { &dev_attr_hw_rev, &dev_attr_board_id, - &dev_attr_version, &dev_attr_nctxts, &dev_attr_nfreectxts, &dev_attr_serial, &dev_attr_boardversion, &dev_attr_tempsense, - &dev_attr_localbus_info, &dev_attr_chip_reset, };