On 12/12/2018 2:59 PM, Leon Romanovsky wrote:
On Wed, Dec 12, 2018 at 02:02:15PM -0500, Dennis Dalessandro wrote:
On 12/12/2018 1:53 PM, Leon Romanovsky wrote:
On Wed, Dec 12, 2018 at 01:45:39PM -0500, Dennis Dalessandro wrote:
On 12/11/2018 1:27 PM, Leon Romanovsky wrote:
On Tue, Dec 11, 2018 at 12:21:53PM -0500, Dennis Dalessandro wrote:
On 12/11/2018 8:09 AM, Leon Romanovsky wrote:
On Tue, Dec 11, 2018 at 07:03:46AM -0500, Dennis Dalessandro wrote:
On 12/11/2018 6:36 AM, Yuval Shaia wrote:
On Tue, Dec 11, 2018 at 12:06:30PM +0200, Leon Romanovsky wrote:
From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
Update hfi1 to use PCI-IDs as an identification in debugfs, instead
of device name which can be changed after user executes device rename.
Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
---
drivers/infiniband/hw/hfi1/debugfs.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/hw/hfi1/debugfs.c b/drivers/infiniband/hw/hfi1/debugfs.c
index 0a557795563c..54b579d8ec07 100644
--- a/drivers/infiniband/hw/hfi1/debugfs.c
+++ b/drivers/infiniband/hw/hfi1/debugfs.c
@@ -1166,23 +1166,24 @@ void hfi1_dbg_ibdev_init(struct hfi1_ibdev *ibd)
char name[sizeof("port0counters") + 1];
char link[10];
struct hfi1_devdata *dd = dd_from_dev(ibd);
+ struct pci_dev *pdev = dd->pcidev;
struct hfi1_pportdata *ppd;
int unit = dd->unit;
int i, j;
if (!hfi1_dbg_root)
return;
- snprintf(name, sizeof(name), "%s_%d", class_name(), unit);
+
snprintf(link, sizeof(link), "%d", unit);
- ibd->hfi1_ibdev_dbg = debugfs_create_dir(name, hfi1_dbg_root);
+ ibd->hfi1_ibdev_dbg = debugfs_create_dir(pci_name(pdev), hfi1_dbg_root);
if (!ibd->hfi1_ibdev_dbg) {
- pr_warn("create of %s failed\n", name);
+ pr_warn("create of %s failed\n", pci_name(pdev));
return;
}
ibd->hfi1_ibdev_link =
- debugfs_create_symlink(link, hfi1_dbg_root, name);
+ debugfs_create_symlink(link, hfi1_dbg_root, pci_name(pdev));
if (!ibd->hfi1_ibdev_link) {
- pr_warn("create of %s symlink failed\n", name);
+ pr_warn("create of %s symlink failed\n", pci_name(pdev));
return;
}
DEBUGFS_SEQ_FILE_CREATE(opcode_stats, ibd->hfi1_ibdev_dbg, ibd);
Reviewed-by: Yuval Shaia <yuval.shaia@xxxxxxxxxx>
Nak for now. This isn't quite what we have in mind. Patch is on the way,
going through review and testing. Will post soon to for-next.
Awesome, can you shed the light and summarize why this patch isn't good
fit for you?
The patch is very similar, our version, which is part of a bigger series,
doesn't use local variable and changes the pr_warn() calls to dd_dev_warn().
I can go ahead and post our version, or you could make a v2 to fix the
pr_warns, up to you. The local variable vs non isn't a big concern here,
meaning I'm fine either way.
I tried to localize and minimize my changes, so it is better to go with
your more comprehensive variant.
During review an issue was raised that this impacts a number of user space
scripts/tools and there is a desire to not break the existing ABI and
instead maintain a separate name space here. Considering device rename
applies to the ib verbs device not necessarily what we have in debugfs which
applies to the HW.
Dennis,
We had this discussion more than once and the overall agreement was that
debugfs can't be qualified as ABI and breakage is expected.
So shall I understand from your response that your internal patch won't
come and we shall proceed with my version?
Which is why I said there is a desire to not break it, rather than a
requirement to not break it. It's under discussion still.
No we should not proceed with your version, whether our internal patch is
posted or not. Is there a compelling reason why what we do in our debugfs
stuff matters that much when it comes to verbs renaming?
Because it is not related to verbs at all, but general core
infrastructure change needed for all devices in RDMA subsystem,
which are using struct ib_device to hold data.
Core, verbs, whatever. Point is that debugfs is still something
different and doesn't have to be subject to the same naming scheme.
Including your hfi1 bug, which can be solved by udev/systemd rule now.
https://patchwork.kernel.org/patch/9508879/
Yes, well aware, and do agree. It comes down to what's better for end
users, that's what we are debating about.
-Denny
