[PATCH] thunderbolt: debugfs: Show all accessible dwords

[PATCH] thunderbolt: debugfs: Show all accessible dwords

[Gil Fine]

Currently, when first failure occurs while reading of the block,
we stop reading the block and jump to the next capability.
This doesn't cover the case of block with "holes" of inaccessible
dwords, followed by accessible dwords.
This patch address this problem.
In case of failure while reading the complete block in one transaction,
(because of one or more dwords is inaccessible), we read the remaining
dwords of the block dword-by-dword, one dword per transaction,
till the end of the block.
By doing this, we handle the case of block with "holes" of inaccessible
dwords, followed by accessible dwords. The accessible dwords are shown
with the fields: <offset> <relative_offset> <cap_id> <vs_cap_id> <value>
E.g.:
0x01eb  236 0x05 0x06 0x0000d166
While the inaccesible dwords are shown as: <offset> <not accessible>
E.g.:
0x01ed <not accessible>

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/debugfs.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/thunderbolt/debugfs.c b/drivers/thunderbolt/debugfs.c
index 201036507cb8..c850b0ac098c 100644
--- a/drivers/thunderbolt/debugfs.c
+++ b/drivers/thunderbolt/debugfs.c
@@ -265,10 +265,8 @@ static void cap_show_by_dw(struct seq_file *s, struct tb_switch *sw,
 		else
 			ret = tb_sw_read(sw, &data, TB_CFG_SWITCH, cap + offset + i, 1);
 		if (ret) {
-			seq_printf(s, "0x%04x <not accessible>\n", cap + offset);
-			if (dwords - i > 1)
-				seq_printf(s, "0x%04x ...\n", cap + offset + 1);
-			return;
+			seq_printf(s, "0x%04x <not accessible>\n", cap + offset + i);
+			continue;
 		}
 
 		seq_printf(s, "0x%04x %4d 0x%02x 0x%02x 0x%08x\n", cap + offset + i,
@@ -292,7 +290,7 @@ static void cap_show(struct seq_file *s, struct tb_switch *sw,
 		else
 			ret = tb_sw_read(sw, data, TB_CFG_SWITCH, cap + offset, dwords);
 		if (ret) {
-			cap_show_by_dw(s, sw, port, cap, offset, cap_id, vsec_id, dwords);
+			cap_show_by_dw(s, sw, port, cap, offset, cap_id, vsec_id, length);
 			return;
 		}
 
-- 
2.17.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH] thunderbolt: debugfs: Show all accessible dwords

[Yehezkel Bernat]

On Tue, Mar 9, 2021 at 11:22 AM Gil Fine <gil.fine@intel.com> wrote:
>
> Currently, when first failure occurs while reading of the block,
> we stop reading the block and jump to the next capability.
> This doesn't cover the case of block with "holes" of inaccessible
> dwords, followed by accessible dwords.
> This patch address this problem.
> In case of failure while reading the complete block in one transaction,
> (because of one or more dwords is inaccessible), we read the remaining
> dwords of the block dword-by-dword, one dword per transaction,
> till the end of the block.

Just wondering: is there any chance this will slow down the device adding flow
if there are many inaccessible dwords, or we expect the read to return
immediately for such dwords?  If for some devices it may be a slow process, does
it make sense to have a way to disable this feature for specific devices or by a
module parameter?  Maybe we don't worry about it as it's only for debugfs?

Thanks,
Yehezkel

Re: [PATCH] thunderbolt: debugfs: Show all accessible dwords

[Mika Westerberg]

On Tue, Mar 09, 2021 at 12:47:11PM +0200, Yehezkel Bernat wrote:
> On Tue, Mar 9, 2021 at 11:22 AM Gil Fine <gil.fine@intel.com> wrote:
> >
> > Currently, when first failure occurs while reading of the block,
> > we stop reading the block and jump to the next capability.
> > This doesn't cover the case of block with "holes" of inaccessible
> > dwords, followed by accessible dwords.
> > This patch address this problem.
> > In case of failure while reading the complete block in one transaction,
> > (because of one or more dwords is inaccessible), we read the remaining
> > dwords of the block dword-by-dword, one dword per transaction,
> > till the end of the block.
> 
> Just wondering: is there any chance this will slow down the device adding flow
> if there are many inaccessible dwords, or we expect the read to return
> immediately for such dwords?  If for some devices it may be a slow process, does
> it make sense to have a way to disable this feature for specific devices or by a
> module parameter?  Maybe we don't worry about it as it's only for debugfs?

We don't worry about it as it is just debugfs ;-)

Re: [PATCH] thunderbolt: debugfs: Show all accessible dwords

[Mika Westerberg]

On Tue, Mar 09, 2021 at 11:23:30AM +0200, Gil Fine wrote:
> Currently, when first failure occurs while reading of the block,
> we stop reading the block and jump to the next capability.
> This doesn't cover the case of block with "holes" of inaccessible
> dwords, followed by accessible dwords.
> This patch address this problem.
> In case of failure while reading the complete block in one transaction,
> (because of one or more dwords is inaccessible), we read the remaining
> dwords of the block dword-by-dword, one dword per transaction,
> till the end of the block.
> By doing this, we handle the case of block with "holes" of inaccessible
> dwords, followed by accessible dwords. The accessible dwords are shown
> with the fields: <offset> <relative_offset> <cap_id> <vs_cap_id> <value>
> E.g.:
> 0x01eb  236 0x05 0x06 0x0000d166
> While the inaccesible dwords are shown as: <offset> <not accessible>
> E.g.:
> 0x01ed <not accessible>
> 
> Signed-off-by: Gil Fine <gil.fine@intel.com>

Applied to thunderbolt.git/next, thanks!

Re: [PATCH] thunderbolt: debugfs: Show all accessible dwords

[Prashant Malani]

Hi Gil,


On Tue, Mar 9, 2021 at 1:23 AM Gil Fine <gil.fine@intel.com> wrote:
>
> Currently, when first failure occurs while reading of the block,
> we stop reading the block and jump to the next capability.
> This doesn't cover the case of block with "holes" of inaccessible
> dwords, followed by accessible dwords.
> This patch address this problem.
> In case of failure while reading the complete block in one transaction,
> (because of one or more dwords is inaccessible), we read the remaining
> dwords of the block dword-by-dword, one dword per transaction,
> till the end of the block.
> By doing this, we handle the case of block with "holes" of inaccessible
> dwords, followed by accessible dwords. The accessible dwords are shown
> with the fields: <offset> <relative_offset> <cap_id> <vs_cap_id> <value>
> E.g.:
> 0x01eb  236 0x05 0x06 0x0000d166
> While the inaccesible dwords are shown as: <offset> <not accessible>
> E.g.:
> 0x01ed <not accessible>
>
> Signed-off-by: Gil Fine <gil.fine@intel.com>
> ---
>  drivers/thunderbolt/debugfs.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/thunderbolt/debugfs.c b/drivers/thunderbolt/debugfs.c
> index 201036507cb8..c850b0ac098c 100644
> --- a/drivers/thunderbolt/debugfs.c
> +++ b/drivers/thunderbolt/debugfs.c
> @@ -265,10 +265,8 @@ static void cap_show_by_dw(struct seq_file *s, struct tb_switch *sw,
>                 else
>                         ret = tb_sw_read(sw, &data, TB_CFG_SWITCH, cap + offset + i, 1);
>                 if (ret) {
> -                       seq_printf(s, "0x%04x <not accessible>\n", cap + offset);
> -                       if (dwords - i > 1)
> -                               seq_printf(s, "0x%04x ...\n", cap + offset + 1);
> -                       return;
> +                       seq_printf(s, "0x%04x <not accessible>\n", cap + offset + i);
> +                       continue;
>                 }
>
>                 seq_printf(s, "0x%04x %4d 0x%02x 0x%02x 0x%08x\n", cap + offset + i,
> @@ -292,7 +290,7 @@ static void cap_show(struct seq_file *s, struct tb_switch *sw,
>                 else
>                         ret = tb_sw_read(sw, data, TB_CFG_SWITCH, cap + offset, dwords);
>                 if (ret) {
> -                       cap_show_by_dw(s, sw, port, cap, offset, cap_id, vsec_id, dwords);

Sorry for being late here:
Can we call cap_show_by_dw(..., dwords) directly here, instead of
having the logic of doing the block read and then resorting to this if
the block read fails?
Since it's debugfs, I doubt efficiency gains are paramount here, and
we have a simpler invocation at the callsite.

> +                       cap_show_by_dw(s, sw, port, cap, offset, cap_id, vsec_id, length);
>                         return;
>                 }
>
> --
> 2.17.1
>
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>

Re: [PATCH] thunderbolt: debugfs: Show all accessible dwords

[Mika Westerberg]

Hi,

On Wed, Mar 17, 2021 at 05:49:33PM -0700, Prashant Malani wrote:
> Hi Gil,
> 
> 
> On Tue, Mar 9, 2021 at 1:23 AM Gil Fine <gil.fine@intel.com> wrote:
> >
> > Currently, when first failure occurs while reading of the block,
> > we stop reading the block and jump to the next capability.
> > This doesn't cover the case of block with "holes" of inaccessible
> > dwords, followed by accessible dwords.
> > This patch address this problem.
> > In case of failure while reading the complete block in one transaction,
> > (because of one or more dwords is inaccessible), we read the remaining
> > dwords of the block dword-by-dword, one dword per transaction,
> > till the end of the block.
> > By doing this, we handle the case of block with "holes" of inaccessible
> > dwords, followed by accessible dwords. The accessible dwords are shown
> > with the fields: <offset> <relative_offset> <cap_id> <vs_cap_id> <value>
> > E.g.:
> > 0x01eb  236 0x05 0x06 0x0000d166
> > While the inaccesible dwords are shown as: <offset> <not accessible>
> > E.g.:
> > 0x01ed <not accessible>
> >
> > Signed-off-by: Gil Fine <gil.fine@intel.com>
> > ---
> >  drivers/thunderbolt/debugfs.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/debugfs.c b/drivers/thunderbolt/debugfs.c
> > index 201036507cb8..c850b0ac098c 100644
> > --- a/drivers/thunderbolt/debugfs.c
> > +++ b/drivers/thunderbolt/debugfs.c
> > @@ -265,10 +265,8 @@ static void cap_show_by_dw(struct seq_file *s, struct tb_switch *sw,
> >                 else
> >                         ret = tb_sw_read(sw, &data, TB_CFG_SWITCH, cap + offset + i, 1);
> >                 if (ret) {
> > -                       seq_printf(s, "0x%04x <not accessible>\n", cap + offset);
> > -                       if (dwords - i > 1)
> > -                               seq_printf(s, "0x%04x ...\n", cap + offset + 1);
> > -                       return;
> > +                       seq_printf(s, "0x%04x <not accessible>\n", cap + offset + i);
> > +                       continue;
> >                 }
> >
> >                 seq_printf(s, "0x%04x %4d 0x%02x 0x%02x 0x%08x\n", cap + offset + i,
> > @@ -292,7 +290,7 @@ static void cap_show(struct seq_file *s, struct tb_switch *sw,
> >                 else
> >                         ret = tb_sw_read(sw, data, TB_CFG_SWITCH, cap + offset, dwords);
> >                 if (ret) {
> > -                       cap_show_by_dw(s, sw, port, cap, offset, cap_id, vsec_id, dwords);
> 
> Sorry for being late here:
> Can we call cap_show_by_dw(..., dwords) directly here, instead of
> having the logic of doing the block read and then resorting to this if
> the block read fails?
> Since it's debugfs, I doubt efficiency gains are paramount here, and
> we have a simpler invocation at the callsite.

I don't think that complicates too much, and in most cases we don't
expect the cap read to fail so doing block read saves us sending
multiple messages over the fabric (even if we don't care about the
performance here).

In any case the patch is already applied so if you want to change this
please send an incremental patch.