On Wed, Dec 12, 2018 at 03:05:59PM -0500, Dennis Dalessandro wrote:
> 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.
Everything is different, but it doesn't mean that we should sacrifice
uniformed user experience across whole subsystem.
>
> > 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.
I'm glad to hear.
The other possible solution is to disable device rename for hfi1, because once we
introduce addition to udev/systemd, which will ensure persistent naming (device
renaming will be done on boot), the whole hfi1 debugfs will break badly. So it
is better to fix or to disable now.
Thanks
>
> -Denny
>
Attachment:
signature.asc
Description: PGP signature
