Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Guido Günther]

Hi ,
On Sat, Feb 13, 2021 at 11:12:37AM +0800, kernel test robot wrote:
> Hi "Guido,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on usb/usb-testing]
> [also build test WARNING on v5.11-rc7 next-20210211]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Guido-G-nther/usb-typec-tps6598x-Add-IRQ-flag-and-register-tracing/20210212-200855
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> config: openrisc-randconfig-s032-20210209 (attached as .config)
> compiler: or1k-linux-gcc (GCC) 9.3.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # apt-get install sparse
>         # sparse version: v0.6.3-215-g0fb77bb6-dirty
>         # https://github.com/0day-ci/linux/commit/ba45e1d5e1fd25b6aed8724106e6c7d5adef7a20
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Guido-G-nther/usb-typec-tps6598x-Add-IRQ-flag-and-register-tracing/20210212-200855
>         git checkout ba45e1d5e1fd25b6aed8724106e6c7d5adef7a20
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=openrisc 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> 
> 
> "sparse warnings: (new ones prefixed by >>)"
>    drivers/usb/typec/tps6598x.c: note: in included file (through include/trace/trace_events.h, include/trace/define_trace.h, drivers/usb/typec/tps6598x_trace.h):
> >> drivers/usb/typec/./tps6598x_trace.h:157:1: sparse: sparse: too long token expansion
> 

I looked around but didn't find any hints how to fix this. Any pointers
I missed (added the sparse list to cc:)?

Cheers,
 -- Guido

> vim +157 drivers/usb/typec/./tps6598x_trace.h
> 
> c90c0282e4ce33 Guido Günther 2021-02-12  156  
> ba45e1d5e1fd25 Guido Günther 2021-02-12 @157  TRACE_EVENT(tps6598x_status,
> ba45e1d5e1fd25 Guido Günther 2021-02-12  158  	    TP_PROTO(u32 status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  159  	    TP_ARGS(status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  160  
> ba45e1d5e1fd25 Guido Günther 2021-02-12  161  	    TP_STRUCT__entry(
> ba45e1d5e1fd25 Guido Günther 2021-02-12  162  			     __field(u32, status)
> ba45e1d5e1fd25 Guido Günther 2021-02-12  163  			     ),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  164  
> ba45e1d5e1fd25 Guido Günther 2021-02-12  165  	    TP_fast_assign(
> ba45e1d5e1fd25 Guido Günther 2021-02-12  166  			   __entry->status = status;
> ba45e1d5e1fd25 Guido Günther 2021-02-12  167  			   ),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  168  
> ba45e1d5e1fd25 Guido Günther 2021-02-12  169  	    TP_printk("conn: %s, pp_5v0: %s, pp_hv: %s, pp_ext: %s, pp_cable: %s, "
> ba45e1d5e1fd25 Guido Günther 2021-02-12  170  		      "pwr-src: %s, vbus: %s, usb-host: %s, legacy: %s, flags: %s",
> ba45e1d5e1fd25 Guido Günther 2021-02-12  171  		      show_status_conn_state(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  172  		      show_status_pp_switch_state(TPS_STATUS_PP_5V0_SWITCH(__entry->status)),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  173  		      show_status_pp_switch_state(TPS_STATUS_PP_HV_SWITCH(__entry->status)),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  174  		      show_status_pp_switch_state(TPS_STATUS_PP_EXT_SWITCH(__entry->status)),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  175  		      show_status_pp_switch_state(TPS_STATUS_PP_CABLE_SWITCH(__entry->status)),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  176  		      show_status_power_sources(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  177  		      show_status_vbus_status(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  178  		      show_status_usb_host_present(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  179  		      show_status_legacy(__entry->status),
> ba45e1d5e1fd25 Guido Günther 2021-02-12  180  		      show_status_flags(__entry->status)
> ba45e1d5e1fd25 Guido Günther 2021-02-12  181  		    )
> ba45e1d5e1fd25 Guido Günther 2021-02-12  182  );
> ba45e1d5e1fd25 Guido Günther 2021-02-12  183  
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org


> _______________________________________________
> kbuild mailing list -- kbuild@lists.01.org
> To unsubscribe send an email to kbuild-leave@lists.01.org

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Ramsay Jones]

[trimmed CC: list]

On 14/02/2021 17:06, Guido Günther wrote:
[snip]
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <rong.a.chen@intel.com>
>>
>>
>> "sparse warnings: (new ones prefixed by >>)"
>>    drivers/usb/typec/tps6598x.c: note: in included file (through include/trace/trace_events.h, include/trace/define_trace.h, drivers/usb/typec/tps6598x_trace.h):
>>>> drivers/usb/typec/./tps6598x_trace.h:157:1: sparse: sparse: too long token expansion
>>
> 
> I looked around but didn't find any hints how to fix this. Any pointers
> I missed (added the sparse list to cc:)?

This is a limitation of sparse; when using the 'stringize' pre-processor
operator #, the maximum size of the resulting string is about 8k (if I
remember correctly).

It just so happens that I have a WIP patch from 2011 hanging around to
fix this - I didn't submit the patch, since it was easier to change the
original source (so that it didn't need such a huge string!). ;-)

Also, it was a very 'quick-n-dirty' patch which could probably be somewhat
improved (and may not even apply to the current codebase*).

ATB,
Ramsay Jones

*) I just tried to apply the patch but, as expected, there are conflicts.

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Linus Torvalds]

On Sun, Feb 14, 2021 at 10:42 AM Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> >
> > I looked around but didn't find any hints how to fix this. Any pointers
> > I missed (added the sparse list to cc:)?
>
> This is a limitation of sparse; when using the 'stringize' pre-processor
> operator #, the maximum size of the resulting string is about 8k (if I
> remember correctly).

Well, yes and no.

The C89 standard actually says that a string literal can be at most
509 characters to be portable. C99 increased it to 4095 characters.

Sparse makes the limit higher, and the limit could easily be expanded
way past 8kB - but the point is that large string literals are
actually not guaranteed to be valid C.

So honestly, it really sounds like that TRACE_EVENT() thing is doing
something it shouldn't be doing.

I don't think there's any fundamental limit why sparse does 8kB as a
limit (just a few random buffers). Making sparse accept larger ones
should be as simple as just increasing MAX_STRING, but I really don't
think the kernel should encourage that kind of excessive string sizes.

I wouldn't be surprised if tracing buffers etc make such strings useless anyway.

               Linus

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Linus Torvalds]

On Sun, Feb 14, 2021 at 11:00 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The C89 standard actually says that a string literal can be at most
> 509 characters to be portable. C99 increased it to 4095 characters.
>
> Sparse makes the limit higher, and the limit could easily be expanded
> way past 8kB - but the point is that large string literals are
> actually not guaranteed to be valid C.

Looking around, there's a couple of other similar cases:

  drivers/infiniband/hw/hfi1/./trace_tx.h:727:1: error: too long token expansion
  arch/x86/purgatory/kexec-purgatory.c:1340:9: warning: trying to
concatenate 21400-character string (8191 bytes max)
  drivers/scsi/constants.c:318:9: warning: trying to concatenate
26550-character string (8191 bytes max)
  kernel/trace/trace.c:5290:1: warning: trying to concatenate
10842-character string (8191 bytes max)

but those four are the only ones I see from a quick x86-64 allmodconfig build.

Please try to avoid it.

          Linus

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Ramsay Jones]

On 14/02/2021 19:00, Linus Torvalds wrote:
> On Sun, Feb 14, 2021 at 10:42 AM Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>>
>>>
>>> I looked around but didn't find any hints how to fix this. Any pointers
>>> I missed (added the sparse list to cc:)?
>>
>> This is a limitation of sparse; when using the 'stringize' pre-processor
>> operator #, the maximum size of the resulting string is about 8k (if I
>> remember correctly).
> 
> Well, yes and no.
> 
> The C89 standard actually says that a string literal can be at most
> 509 characters to be portable. C99 increased it to 4095 characters.
> 
> Sparse makes the limit higher, and the limit could easily be expanded
> way past 8kB - but the point is that large string literals are
> actually not guaranteed to be valid C.
> 
> So honestly, it really sounds like that TRACE_EVENT() thing is doing
> something it shouldn't be doing.

Yep, as I said, I didn't submit the patch - rather I changed the source
so as not to need such a long string.

> I don't think there's any fundamental limit why sparse does 8kB as a
> limit (just a few random buffers). Making sparse accept larger ones
> should be as simple as just increasing MAX_STRING, but I really don't
> think the kernel should encourage that kind of excessive string sizes.

I agree, but I wiggled my patch (which doesn't increase MAX_STRING) to
apply to the current codebase, and ... it now fails two tests! ;-)
(It seems, in the intervening 9 years, the show_token_sequence() function
fixed the quoting of double-quotes in the resulting strings, which
my patch fails to do).

Sorry for the noise.

ATB,
Ramsay Jones

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Luc Van Oostenryck]

On Sun, Feb 14, 2021 at 11:00:48AM -0800, Linus Torvalds wrote:
> On Sun, Feb 14, 2021 at 10:42 AM Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
> >
> > >
> > > I looked around but didn't find any hints how to fix this. Any pointers
> > > I missed (added the sparse list to cc:)?
> >
> > This is a limitation of sparse; when using the 'stringize' pre-processor
> > operator #, the maximum size of the resulting string is about 8k (if I
> > remember correctly).
> 
> Well, yes and no.
> 
> The C89 standard actually says that a string literal can be at most
> 509 characters to be portable. C99 increased it to 4095 characters.
> 
> Sparse makes the limit higher, and the limit could easily be expanded
> way past 8kB - but the point is that large string literals are
> actually not guaranteed to be valid C.
> 
> So honestly, it really sounds like that TRACE_EVENT() thing is doing
> something it shouldn't be doing.

In itself, it's OKish but it does a lot of macro expansions and most
arguments are macros of macros of ... but the problem seems to be
limited to TP_printk().

In the current case, the offender is the string 'print_fmt_tps6598x_status'
which is just under 26K long especially because it expand
TPS6598X_STATUS_FLAGS_MASK but also because the arguments use FIELD_GET()
and thus __BF_FIELD_CHECK().
> 
> I don't think there's any fundamental limit why sparse does 8kB as a
> limit (just a few random buffers). Making sparse accept larger ones
> should be as simple as just increasing MAX_STRING, but I really don't
> think the kernel should encourage that kind of excessive string sizes.

Like you noted, there are just a few cases in the kernel and IIRC
there is or was one case in it too.
I would tend to increase MAX_STRING to something like 32 or 64K,
in order to keep it reasonable but let sparse to continue its processing,
but add a warning when the string/token is bigger than the current 8K.

-- Luc

Re: [PATCH v2 2/4] usb: typec: tps6598x: Add trace event for status register

[Guido Günther]

Hi,
On Sun, Feb 14, 2021 at 09:41:27PM +0100, Luc Van Oostenryck wrote:
> On Sun, Feb 14, 2021 at 11:00:48AM -0800, Linus Torvalds wrote:
> > On Sun, Feb 14, 2021 at 10:42 AM Ramsay Jones
> > <ramsay@ramsayjones.plus.com> wrote:
> > >
> > > >
> > > > I looked around but didn't find any hints how to fix this. Any pointers
> > > > I missed (added the sparse list to cc:)?
> > >
> > > This is a limitation of sparse; when using the 'stringize' pre-processor
> > > operator #, the maximum size of the resulting string is about 8k (if I
> > > remember correctly).
> > 
> > Well, yes and no.
> > 
> > The C89 standard actually says that a string literal can be at most
> > 509 characters to be portable. C99 increased it to 4095 characters.
> > 
> > Sparse makes the limit higher, and the limit could easily be expanded
> > way past 8kB - but the point is that large string literals are
> > actually not guaranteed to be valid C.
> > 
> > So honestly, it really sounds like that TRACE_EVENT() thing is doing
> > something it shouldn't be doing.
> 
> In itself, it's OKish but it does a lot of macro expansions and most
> arguments are macros of macros of ... but the problem seems to be
> limited to TP_printk().
> 
> In the current case, the offender is the string 'print_fmt_tps6598x_status'
> which is just under 26K long especially because it expand
> TPS6598X_STATUS_FLAGS_MASK but also because the arguments use FIELD_GET()
> and thus __BF_FIELD_CHECK().

That was a great hint! Using a custom FIELD_GET() that drops the
__BF_FIELD_CHECK() makes things fit.
Cheers,
 -- Guido

> > 
> > I don't think there's any fundamental limit why sparse does 8kB as a
> > limit (just a few random buffers). Making sparse accept larger ones
> > should be as simple as just increasing MAX_STRING, but I really don't
> > think the kernel should encourage that kind of excessive string sizes.
> 
> Like you noted, there are just a few cases in the kernel and IIRC
> there is or was one case in it too.
> I would tend to increase MAX_STRING to something like 32 or 64K,
> in order to keep it reasonable but let sparse to continue its processing,
> but add a warning when the string/token is bigger than the current 8K.
> 
> -- Luc
>