Re: [PATCH v4] kobject: Replace strlcpy with strscpy
On Thu, 31 Aug 2023 14:01:04 +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > [...] Applied to for-next/hardening, thanks! [1/1] kobject: Replace strlcpy with strscpy https://git.kernel.org/kees/c/68a39dfd6f94 Take care, -- Kees Cook
Re: [PATCH] init/version.c: Replace strlcpy with strscpy
On Wed, 30 Aug 2023 16:08:06 +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > [...] Applied to for-next/hardening, thanks! [1/1] init/version.c: Replace strlcpy with strscpy https://git.kernel.org/kees/c/ec23bc09c1c0 Take care, -- Kees Cook
Re: [PATCH] HID: uhid: refactor deprecated strncpy
On Thu, Sep 14, 2023 at 10:47:21PM +, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We should prefer more robust and less ambiguous string interfaces.
>
> A suitable replacement is `strscpy` [2] due to the fact that it
> guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
>
> Looking at: Commit 4d26d1d1e806 ("Revert "HID: uhid: use strlcpy() instead of
> strncpy()"")
> we see referenced the fact that many attempts have been made to change
> these strncpy's into strlcpy to no success. I think strscpy is an
> objectively better interface here as it doesn't unnecessarily NUL-pad
> the destination buffer whilst allowing us to drop the `len = min(...)`
> pattern as strscpy will implicitly limit the number of bytes copied by
> the smaller of its dest and src arguments.
I think the worry here is that ev->u.create2.name may not be %NUL
terminated. But I think that doesn't matter, see below...
Also, note, this should CC David Herrmann
(now added).
> So while the existing code may not have a bug (i.e: overread problems)
> we should still favor strscpy due to readability (plus a very slight
> performance boost).
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> ---
> drivers/hid/uhid.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 4588d2cd4ea4..00e1566ad37b 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -490,7 +490,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
> const struct uhid_event *ev)
> {
> struct hid_device *hid;
> - size_t rd_size, len;
> + size_t rd_size;
> void *rd_data;
> int ret;
>
> @@ -514,13 +514,9 @@ static int uhid_dev_create2(struct uhid_device *uhid,
> goto err_free;
> }
>
> - /* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
> - len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
> - strncpy(hid->name, ev->u.create2.name, len);
> - len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
> - strncpy(hid->phys, ev->u.create2.phys, len);
> - len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
> - strncpy(hid->uniq, ev->u.create2.uniq, len);
ev->u.create2 is:
struct uhid_create2_req {
__u8 name[128];
__u8 phys[64];
__u8 uniq[64];
...
hid is:
struct hid_device { /* device report descriptor */
...
char name[128]; /* Device name */
char phys[64]; /* Device physical location */
char uniq[64]; /* Device unique identifier (serial #) */
So these "min" calls are redundant -- it wants to copy at most 1 less so
it can be %NUL terminated. Which is what strscpy() already does. And
source and dest are the same size, so we can't over-read source if it
weren't terminated (since strscpy won't overread like strlcpy).
> + strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
> + strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
> + strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));
Reviewed-by: Kees Cook
-Kees
>
> hid->ll_driver = _hid_driver;
> hid->bus = ev->u.create2.bus;
>
> ---
> base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
> change-id: 20230914-strncpy-drivers-hid-uhid-c-a465f3e784dd
>
> Best regards,
> --
> Justin Stitt
>
--
Kees Cook
Re: [PATCH] HID: prodikeys: refactor deprecated strncpy
On Thu, Sep 14, 2023 at 10:20:55PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer without unnecessarily NUL-padding. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt > --- > Note: for some reason if NUL-padding is needed let's opt for `strscpy_pad()` > --- > drivers/hid/hid-prodikeys.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c > index e4e9471d0f1e..c16d2ba6ea16 100644 > --- a/drivers/hid/hid-prodikeys.c > +++ b/drivers/hid/hid-prodikeys.c > @@ -639,9 +639,9 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) > goto fail; > } > > - strncpy(card->driver, shortname, sizeof(card->driver)); > - strncpy(card->shortname, shortname, sizeof(card->shortname)); > - strncpy(card->longname, longname, sizeof(card->longname)); > + strscpy(card->driver, shortname, sizeof(card->driver)); > + strscpy(card->shortname, shortname, sizeof(card->shortname)); > + strscpy(card->longname, longname, sizeof(card->longname)); "card" is already kzalloc()ed so no _pad() is needed, good. > > /* Set up rawmidi */ > err = snd_rawmidi_new(card, card->shortname, 0, > @@ -652,7 +652,7 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) > goto fail; > } > pm->rwmidi = rwmidi; > - strncpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); > + strscpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); > rwmidi->info_flags = SNDRV_RAWMIDI_INFO_INPUT; > rwmidi->private_data = pm; Same here. Reviewed-by: Kees Cook -Kees > > > --- > base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec > change-id: 20230914-strncpy-drivers-hid-hid-prodikeys-c-cf42614a21d4 > > Best regards, > -- > Justin Stitt > -- Kees Cook
Re: [PATCH] firmware: ti_sci: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 08:23:02PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer. > > It does not seem like `ver->firmware_description` requires NUL-padding > (which is a behavior that strncpy provides) but if it does let's opt for > `strscpy_pad()`. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt Looks right to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] firmware: tegra: bpmp: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 07:38:44PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > It seems like the filename stored at `namevirt` is expected to be > NUL-terminated. This took me a bit to establish, but yes: buf[256] is used to store filename, so it'll always be %NUL-terminated with the 256 bytes, which is the same size used to allocate virtname, which means it will always be %NUL-terminated. > > A suitable replacement is `strscpy_pad` due to the fact that it > guarantees NUL-termination on the destination buffer whilst maintaining > the NUL-padding behavior that strncpy provides. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt This one looks weird because namevirt seems unused, but I assume there's some kind of DMA side-effect happening somewhere? But, yes, after digging around here, I think this all looks right. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v3] EDAC/mc_sysfs: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 05:20:50PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on the destination buffer without needlessly > NUL-padding. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt > --- > Changes in v3: > - prefer strscpy to strscpy_pad (thanks Tony) > - Link to v2: > https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v2-1-2d2e6bd43...@google.com > > Changes in v2: > - included refactor of another strncpy in same file > - Link to v1: > https://lore.kernel.org/r/20230913-strncpy-drivers-edac-edac_mc_sysfs-c-v1-1-d232891b0...@google.com > --- > Note: build-tested only. > --- > drivers/edac/edac_mc_sysfs.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c > index 15f63452a9be..9a5b4bbd8191 100644 > --- a/drivers/edac/edac_mc_sysfs.c > +++ b/drivers/edac/edac_mc_sysfs.c > @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device > *dev, > if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) > return -EINVAL; > > - strncpy(rank->dimm->label, data, copy_count); > - rank->dimm->label[copy_count] = '\0'; > + strscpy(rank->dimm->label, data, copy_count); Hrm, I don't like the use of "copy_count" here -- it's the source length, not the destination buffer size. It is technically safe because it was bounds-checked right before, but now we run the risk of additional truncation since "copy_count" will not include the '\0'. Imagine data = "a", count = 1. strscpy(dst, data, 1) will only copy '\0'. I think this should be memcpy(), not strscpy(). The bounds checking and truncation of '\n' has already been calculated -- we're just doing a byte copy at this point: if (count == 0) return -EINVAL; if (data[count - 1] == '\0' || data[count - 1] == '\n') copy_count -= 1; if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label)) return -EINVAL; memcpy(rank->dimm->label, data, copy_count); rank->dimm->label[copy_count] = '\0'; > > return count; > } > @@ -535,7 +534,7 @@ static ssize_t dimmdev_label_store(struct device *dev, > if (copy_count == 0 || copy_count >= sizeof(dimm->label)) > return -EINVAL; > > - strncpy(dimm->label, data, copy_count); > + strscpy(dimm->label, data, copy_count); > dimm->label[copy_count] = '\0'; Same for this one: replace strncpy with memcpy. -Kees > > return count; > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230913-strncpy-drivers-edac-edac_mc_sysfs-c-e619b00124a3 > > Best regards, > -- > Justin Stitt > -- Kees Cook
Re: [PATCH] dax: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 01:10:24AM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > `dax_id->dev_name` is expected to be NUL-terminated and has been > zero-allocated. > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on the destination buffer. Moreover, due to > `dax_id` being zero-allocated the padding behavior of `strncpy` is not > needed and a simple 1:1 replacement of strncpy -> strscpy should > suffice. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt Looks correct to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] cpuidle: dt: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 12:23:19AM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer. With this, we can also drop > the now unnecessary `CPUIDLE_(NAME|DESC)_LEN - 1` pieces. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt A very regular strncpy/strscpy conversion. :) Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] cpufreq: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 12:07:21AM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > Both `policy->last_governor` and `default_governor` are expected to be > NUL-terminated which is shown by their heavy usage with other string > apis like `strcmp`. > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt All looks sensible to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] bus: fsl-mc: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 10:52:04PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We need to prefer more robust and less ambiguous string interfaces. > > `obj_desc->(type|label)` are expected to be NUL-terminated strings as > per "include/linux/fsl/mc.h +143" > | ... > | * struct fsl_mc_obj_desc - Object descriptor > | * @type: Type of object: NULL terminated string > | ... > > It seems `cmd_params->obj_type` is also expected to be a NUL-terminated > string. > > A suitable replacement is `strscpy_pad` due to the fact that it > guarantees NUL-termination on the destination buffer whilst keeping the > NUL-padding behavior that `strncpy` provides. I see evidence that %NUL padding isn't needed, like this: /* * Mark the obj entry as "invalid", by using the * empty string as obj type: */ obj_desc->type[0] = '\0'; but there's little harm in being conservative and leaving the padding in. > > Padding may not strictly be necessary but let's opt to keep it as this > ensures no functional change. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Cc: Kees Cook > Signed-off-by: Justin Stitt > --- > Note: build-tested > --- > drivers/bus/fsl-mc/dprc.c | 12 > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c > index d129338b8bc0..dd1b5c0fb7e2 100644 > --- a/drivers/bus/fsl-mc/dprc.c > +++ b/drivers/bus/fsl-mc/dprc.c > @@ -450,10 +450,8 @@ int dprc_get_obj(struct fsl_mc_io *mc_io, > obj_desc->ver_major = le16_to_cpu(rsp_params->version_major); > obj_desc->ver_minor = le16_to_cpu(rsp_params->version_minor); > obj_desc->flags = le16_to_cpu(rsp_params->flags); > - strncpy(obj_desc->type, rsp_params->type, 16); > - obj_desc->type[15] = '\0'; > - strncpy(obj_desc->label, rsp_params->label, 16); > - obj_desc->label[15] = '\0'; > + strscpy_pad(obj_desc->type, rsp_params->type, 16); > + strscpy_pad(obj_desc->label, rsp_params->label, 16); I really don't like all the open-coded sizes, but yeah, okay, it's not technically wrong. :) Reviewed-by: Kees Cook -Kees > return 0; > } > EXPORT_SYMBOL_GPL(dprc_get_obj); > @@ -491,8 +489,7 @@ int dprc_set_obj_irq(struct fsl_mc_io *mc_io, > cmd_params->irq_addr = cpu_to_le64(irq_cfg->paddr); > cmd_params->irq_num = cpu_to_le32(irq_cfg->irq_num); > cmd_params->obj_id = cpu_to_le32(obj_id); > - strncpy(cmd_params->obj_type, obj_type, 16); > - cmd_params->obj_type[15] = '\0'; > + strscpy_pad(cmd_params->obj_type, obj_type, 16); > > /* send command to mc*/ > return mc_send_command(mc_io, ); > @@ -564,8 +561,7 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io, > cmd_params = (struct dprc_cmd_get_obj_region *)cmd.params; > cmd_params->obj_id = cpu_to_le32(obj_id); > cmd_params->region_index = region_index; > - strncpy(cmd_params->obj_type, obj_type, 16); > - cmd_params->obj_type[15] = '\0'; > + strscpy_pad(cmd_params->obj_type, obj_type, 16); > > /* send command to mc*/ > err = mc_send_command(mc_io, ); > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230912-strncpy-drivers-bus-fsl-mc-dprc-c-bc7d818386ec > > Best regards, > -- > Justin Stitt > -- Kees Cook
Re: [PATCH][next] checkpatch: add a couple new alloc functions to alloc with multiplies check
On Tue, Sep 12, 2023 at 10:51:22AM -0700, Joe Perches wrote:
> On Tue, 2023-09-12 at 11:04 -0600, Gustavo A. R. Silva wrote:
> > vmalloc() and vzalloc() functions have now 2-factor multiplication
> > argument forms vmalloc_array() and vcalloc(), correspondingly.
>
> > Add alloc-with-multiplies checks for these new functions.
> >
> > Link: https://github.com/KSPP/linux/issues/342
> > Signed-off-by: Gustavo A. R. Silva
> > ---
> > scripts/checkpatch.pl | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 7d16f863edf1..45265d0eee1b 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -7207,17 +7207,19 @@ sub process {
> > "Prefer $3(sizeof(*$1)...) over $3($4...)\n" .
> > $herecurr);
> > }
> >
> > -# check for (kv|k)[mz]alloc with multiplies that could be
> > kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
> > +# check for (kv|k|v)[mz]alloc with multiplies that could be
> > kmalloc_array/kvmalloc_array/vmalloc_array/kvcalloc/kcalloc/vcalloc
> > if ($perl_version_ok &&
> > defined $stat &&
> > - $stat =~
> > /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
> > {
> > + $stat =~
> > /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,?/)
> > {
> > my $oldfunc = $3;
> > my $a1 = $4;
> > my $a2 = $10;
> > my $newfunc = "kmalloc_array";
> > $newfunc = "kvmalloc_array" if ($oldfunc eq "kvmalloc");
> > + $newfunc = "vmalloc_array" if ($oldfunc eq "vmalloc");
> > $newfunc = "kvcalloc" if ($oldfunc eq "kvzalloc");
> > $newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
> > + $newfunc = "vcalloc" if ($oldfunc eq "vzalloc");
> > my $r1 = $a1;
> > my $r2 = $a2;
> > if ($a1 =~ /^sizeof\s*\S/) {
> > @@ -7233,7 +7235,7 @@ sub process {
> > "Prefer $newfunc over $oldfunc with
> > multiply\n" . $herectx) &&
> > $cnt == 1 &&
> > $fix) {
> > - $fixed[$fixlinenr] =~
> > s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> > . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> > + $fixed[$fixlinenr] =~
> > s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> > . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> > }
> > }
> > }
>
> Seems ok. Dunno how many more of these there might be like
> devm_ variants, but maybe it'd be better style to use a hash
> with replacement instead of the repeated if block
>
> Something like:
> ---
> scripts/checkpatch.pl | 26 --
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7d16f863edf1c..bacb63518be14 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -854,6 +854,23 @@ foreach my $entry (keys %deprecated_apis) {
> }
> $deprecated_apis_search = "(?:${deprecated_apis_search})";
>
> +our %alloc_with_multiply_apis = (
> + "kmalloc" => "kmalloc_array",
> + "kvmalloc" => "kvmalloc_array",
> + "vmalloc" => "vmalloc_array",
> + "kvzalloc" => "kvcalloc",
> + "kzalloc" => "kcalloc",
> + "vzalloc" => "vcalloc",
> +);
Yeah, this seems more readable. Gustavo can you send a v2 with this
rework?
Thanks!
-Kees
> +
> +#Create a search pattern for all these strings to speed up a loop below
> +our $alloc_with_multiply_search = "";
> +foreach my $entry (keys %alloc_with_multiply_apis) {
> + $alloc_with_multiply_search .= '|' if ($alloc_with_multiply_search ne
> "");
> + $alloc_with_multiply_search .= $entry;
> +}
> +$alloc_with_multiply_search = "(?:${alloc_with_multiply_search})";
> +
> our $mode_perms_world_writable = qr{
> S_IWUGO |
> S_IWOTH |
> @@ -7210,14 +7227,11 @@ sub process {
> # check for (kv|k)[mz]alloc with multiplies that could be
> kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
> if ($perl_version_ok &&
> defined $stat &&
> - $stat =~
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
> {
> + $stat =~
>
Re: [PATCH v2][next] RDMA/core: Use size_{add,mul}() in calls to struct_size()
On Mon, Sep 11, 2023 at 05:27:59PM -0600, Gustavo A. R. Silva wrote:
> Harden calls to struct_size() with size_add() and size_mul().
Specifically, make sure that open-coded arithmetic cannot cause an
overflow/wraparound. (i.e. it will stay saturated at SIZE_MAX.)
>
> Fixes: 467f432a521a ("RDMA/core: Split port and device counter sysfs
> attributes")
> Fixes: a4676388e2e2 ("RDMA/core: Simplify how the gid_attrs sysfs is created")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
-Kees
> ---
> Changes in v2:
> - Update changelog text: remove the part about binary differences (it
>was added by mistake).
>
> drivers/infiniband/core/sysfs.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> index ee59d7391568..ec5efdc16660 100644
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -903,7 +903,7 @@ alloc_hw_stats_device(struct ib_device *ibdev)
>* Two extra attribue elements here, one for the lifespan entry and
>* one to NULL terminate the list for the sysfs core code
>*/
> - data = kzalloc(struct_size(data, attrs, stats->num_counters + 1),
> + data = kzalloc(struct_size(data, attrs, size_add(stats->num_counters,
> 1)),
> GFP_KERNEL);
> if (!data)
> goto err_free_stats;
> @@ -1009,7 +1009,7 @@ alloc_hw_stats_port(struct ib_port *port, struct
> attribute_group *group)
>* Two extra attribue elements here, one for the lifespan entry and
>* one to NULL terminate the list for the sysfs core code
>*/
> - data = kzalloc(struct_size(data, attrs, stats->num_counters + 1),
> + data = kzalloc(struct_size(data, attrs, size_add(stats->num_counters,
> 1)),
> GFP_KERNEL);
> if (!data)
> goto err_free_stats;
> @@ -1140,7 +1140,7 @@ static int setup_gid_attrs(struct ib_port *port,
> int ret;
>
> gid_attr_group = kzalloc(struct_size(gid_attr_group, attrs_list,
> - attr->gid_tbl_len * 2),
> + size_mul(attr->gid_tbl_len, 2)),
>GFP_KERNEL);
> if (!gid_attr_group)
> return -ENOMEM;
> @@ -1205,8 +1205,8 @@ static struct ib_port *setup_port(struct ib_core_device
> *coredev, int port_num,
> int ret;
>
> p = kvzalloc(struct_size(p, attrs_list,
> - attr->gid_tbl_len + attr->pkey_tbl_len),
> - GFP_KERNEL);
> + size_add(attr->gid_tbl_len,
> attr->pkey_tbl_len)),
> + GFP_KERNEL);
> if (!p)
> return ERR_PTR(-ENOMEM);
> p->ibdev = device;
> --
> 2.34.1
>
--
Kees Cook
Re: [PATCH] auxdisplay: panel: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 08:51:04PM +, Justin Stitt wrote:
> `strncpy` is deprecated and as such we should prefer more robust and
> less ambiguous interfaces.
>
> In this case, all of `press_str`, `repeat_str` and `release_str` are
> explicitly marked as nonstring:
> | struct { /* valid when type == INPUT_TYPE_KBD */
> | char press_str[sizeof(void *) + sizeof(int)] __nonstring;
> | char repeat_str[sizeof(void *) + sizeof(int)] __nonstring;
> | char release_str[sizeof(void *) + sizeof(int)] __nonstring;
> | } kbd;
>
> ... which makes `strtomem_pad` a suitable replacement as it is
> functionally the same whilst being more obvious about its behavior.
Yup, this is exactly what strtomem_pad() was made for. :)
Reviewed-by: Kees Cook
-Kees
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> ---
> Note: build-tested
> ---
> drivers/auxdisplay/panel.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
> index eba04c0de7eb..e20d35bdf5fe 100644
> --- a/drivers/auxdisplay/panel.c
> +++ b/drivers/auxdisplay/panel.c
> @@ -1449,10 +1449,9 @@ static struct logical_input *panel_bind_key(const char
> *name, const char *press,
> key->rise_time = 1;
> key->fall_time = 1;
>
> - strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str));
> - strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str));
> - strncpy(key->u.kbd.release_str, release,
> - sizeof(key->u.kbd.release_str));
> + strtomem_pad(key->u.kbd.press_str, press, '\0');
> + strtomem_pad(key->u.kbd.repeat_str, repeat, '\0');
> + strtomem_pad(key->u.kbd.release_str, release, '\0');
> list_add(>list, _inputs);
> return key;
> }
>
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230911-strncpy-drivers-auxdisplay-panel-c-83bce51f32cb
>
> Best regards,
> --
> Justin Stitt
>
--
Kees Cook
Re: [PATCH] ACPI: OSI: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 08:36:44PM +, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We know `osi->string` is a NUL-terminated string due to its eventual use
> in `acpi_install_interface()` and `acpi_remove_interface()` which expect
> a `acpi_string` which has been specifically typedef'd as:
> | typedef char *acpi_string; /* Null terminated ASCII string */
>
> ... and which also has other string functions used on it like `strlen`.
> Furthermore, padding is not needed in this instance either.
Following the callers, I agree, this doesn't need %NUL padding -- it's
always processed as a regular C string.
Reviewed-by: Kees Cook
-Kees
>
> Due to the reasoning above a suitable replacement is `strscpy` [2] since
> it guarantees NUL-termination on the destination buffer and doesn't
> unnecessarily NUL-pad.
>
> While there is unlikely to be a buffer overread (or other related bug)
> in this case, we should still favor a more robust and less ambiguous
> interface.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> ---
> Note: build-tested
> ---
> drivers/acpi/osi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c
> index d4405e1ca9b9..df9328c850bd 100644
> --- a/drivers/acpi/osi.c
> +++ b/drivers/acpi/osi.c
> @@ -110,7 +110,7 @@ void __init acpi_osi_setup(char *str)
> break;
> } else if (osi->string[0] == '\0') {
> osi->enable = enable;
> - strncpy(osi->string, str, OSI_STRING_LENGTH_MAX);
> + strscpy(osi->string, str, OSI_STRING_LENGTH_MAX);
> break;
> }
> }
>
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230911-strncpy-drivers-acpi-osi-c-c801b7427987
>
> Best regards,
> --
> Justin Stitt
>
--
Kees Cook
Re: [PATCH] xen/efi: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 06:59:31PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > `efi_loader_signature` has space for 4 bytes. We are copying "Xen" (3 bytes) > plus a NUL-byte which makes 4 total bytes. With that being said, there is > currently not a bug with the current `strncpy()` implementation in terms of > buffer overreads but we should favor a more robust string interface > either way. Yeah, this will work. Since this is a u32 destination, I do wonder if strtomem_pad() would be better since we're not really writing a string? But since this is all hard-coded, it doesn't matter. :) Reviewed-by: Kees Cook -Kees > > A suitable replacement is `strscpy` [2] due to the fact that it guarantees > NUL-termination on the destination buffer while being functionally the > same in this case. > > Link: > www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Cc: Kees Cook > Signed-off-by: Justin Stitt > --- > Note: build-tested > --- > arch/x86/xen/efi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c > index 863d0d6b3edc..7250d0e0e1a9 100644 > --- a/arch/x86/xen/efi.c > +++ b/arch/x86/xen/efi.c > @@ -138,7 +138,7 @@ void __init xen_efi_init(struct boot_params *boot_params) > if (efi_systab_xen == NULL) > return; > > - strncpy((char *)_params->efi_info.efi_loader_signature, "Xen", > + strscpy((char *)_params->efi_info.efi_loader_signature, "Xen", > sizeof(boot_params->efi_info.efi_loader_signature)); > boot_params->efi_info.efi_systab = (__u32)__pa(efi_systab_xen); > boot_params->efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> > 32); > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230911-strncpy-arch-x86-xen-efi-c-14292f5a79ee > > Best regards, > -- > Justin Stitt > -- Kees Cook
Re: [PATCH] x86/tdx: refactor deprecated strncpy
On Mon, Sep 11, 2023 at 03:01:16PM -0700, Justin Stitt wrote: > On Mon, Sep 11, 2023 at 11:51 AM Dave Hansen wrote: > > > > On 9/11/23 11:27, Justin Stitt wrote: > > > `strncpy` is deprecated and we should prefer more robust string apis. > > > > I dunno. It actually seems like a pretty good fit here. > > > > > In this case, `message.str` is not expected to be NUL-terminated as it > > > is simply a buffer of characters residing in a union which allows for > > > named fields representing 8 bytes each. There is only one caller of > > > `tdx_panic()` and they use a 59-length string for `msg`: > > > | const char *msg = "TD misconfiguration: SEPT_VE_DISABLE attribute > > > must be set."; > > > > I'm not really following this logic. > > > > We need to do the following: > > > > 1. Make sure not to over write past the end of 'message' > > 2. Make sure not to over read past the end of 'msg' > > 3. Make sure not to leak stack data into the hypercall registers > >in the case of short strings. > > > > strncpy() does #1, #2 and #3 just fine. > > Right, to be clear, I do not suspect a bug in the current > implementation. Rather, let's move towards a less ambiguous interface > for maintainability's sake > > > > > The resulting string does *NOT* need to be NULL-terminated. See the > > comment: > > > > /* VMM assumes '\0' in byte 65, if the message took all 64 bytes */ > > > > Are there cases where strncpy() doesn't NULL-terminate the string other > > than when the buffer is full? > > > > I actually didn't realize that strncpy() pads its output up to the full > > size. I wonder if Kirill used it intentionally or whether he got lucky > > here. :) > > Big reason to use strtomem_pad as it is more obvious about what it does. > > I'd love more thoughts/testing here. This looks like exactly the right conversion: strtomem_pad() will do 1, 2, and 3 (and does it unambiguously and without allowing for a possible-wrong "size" parameter for the destination buffer). Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] um,ethertap: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 01:12:35PM -0700, Justin Stitt wrote: > On Tue, Sep 12, 2023 at 12:36 AM Geert Uytterhoeven > wrote: > > > > Hi Justin, > > > > Thanks for your patch! > > > > On Mon, Sep 11, 2023 at 7:53 PM Justin Stitt wrote: > > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > > > `gate_buf` should always be NUL-terminated and does not require > > > NUL-padding. It is used as a string arg inside an argv array given to > > > > Can you please explain why it does not require NUL-padding? > > It looks like this buffer is passed eventually to a user space > > application, thus possibly leaking uninitialized stack data. > > It looks like it's being passed as a list of command-line arguments in > `run_helper()`. Should this be NUL-padded due to its eventual use in > user space? If we think yes I can send a v2. Thanks for pointing this > out. No, it's passed as a pointer to a string, and the clone call will ultimately make a copy-until-%NUL when building the new process. This doesn't need padding. Reviewed-by: Kees Cook -Kees -- Kees Cook
Re: [PATCH] vt: Fix potential read overflow of kernel memory
On Thu, Aug 31, 2023 at 10:23:10AM -0400, Azeem Shaikh wrote: > Are folks ok with me sending out a v2 for this with a better commit > log that explains the issue? Yes, please do. It should clear up the questions from this thread. :) Thanks! -Kees -- Kees Cook
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On Thu, 14 Sep 2023 15:11:02 +0200
Clément Léger wrote:
> enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
> this will result in a misaligned access on 64 bits architectures since
> set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
> On architecture that do not support misaligned access, this will crash
> the kernel. Align uaddr on unsigned long size to avoid such behavior.
> This bug was found while running kselftests on RISC-V.
>
> Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event
> enablement")
> Signed-off-by: Clément Léger
> ---
> kernel/trace/trace_events_user.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_events_user.c
> b/kernel/trace/trace_events_user.c
> index 6f046650e527..580c0fe4b23e 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm
> *mm,
> bool fixup_fault, int *attempt)
> {
> unsigned long uaddr = enabler->addr;
> - unsigned long *ptr;
> + unsigned long *ptr, bit_offset;
> struct page *page;
> void *kaddr;
> int ret;
> @@ -511,13 +511,19 @@ static int user_event_enabler_write(struct
> user_event_mm *mm,
> }
>
> kaddr = kmap_local_page(page);
> +
> + bit_offset = uaddr & (sizeof(unsigned long) - 1);
> + if (bit_offset) {
> + bit_offset *= 8;
> + uaddr &= ~(sizeof(unsigned long) - 1);
> + }
> ptr = kaddr + (uaddr & ~PAGE_MASK);
>
> /* Update bit atomically, user tracers must be atomic as well */
> if (enabler->event && enabler->event->status)
> - set_bit(ENABLE_BIT(enabler), ptr);
> + set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
> else
> - clear_bit(ENABLE_BIT(enabler), ptr);
> + clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
What we need are generic set_bit_aligned() and clear_bit_aligned(), which align
the ptr
by unsigned long. (I think it should be done in set_bit/clear_bit, for
architecture
which requires aligned access...)
#define LONG_ALIGN_DIFF(p) (p) & (sizeof(long) -1)
#define LONG_ALINGNED(p)(p) & ~(sizeof(long) - 1)
static inline void set_bit_aligned(int bit, unsigned long *ptr)
{
int offs = LONG_ALIGN_DIFF(ptr) * 8;
#ifdef __BIGENDIAN
if (bit >= offs) {
set_bit(bit - offs, LONG_ALIGNED(ptr));
} else {
set_bit(bit + BITS_PER_LONG - offs, LONG_ALIGNED(ptr) + 1);
}
#else
if (bit < BITS_PER_LONG - offs) {
set_bit(bit + offs, LONG_ALIGNED(ptr));
} else {
set_bit(bit - BITS_PER_LONG + offs, LONG_ALIGNED(ptr) + 1);
}
#endif
}
And use it.
Thank you,
>
> kunmap_local(kaddr);
> unpin_user_pages_dirty_lock(, 1, true);
> --
> 2.40.1
>
--
Masami Hiramatsu (Google)
Re: [PATCH 3/8] usb: udc: trace: reduce buffer usage of trace event
On 9/15/2023 10:16 AM, Steven Rostedt wrote: On Fri, 15 Sep 2023 09:11:06 +0800 Linyu Yuan wrote: + snprintf(__s, 9, "ep%d%s", te.address, \ + (te.caps.dir_in && te.caps.dir_out) ? "" : \ + te.caps.dir_in ? "in" : "out"); Note, there's a temp buffer trace_seq 'p' available for use as well. See both include/trace/events/libata.h and include/trace/events/scsi.h: const char *libata_trace_parse_status(struct trace_seq*, unsigned char); #define __parse_status(s) libata_trace_parse_status(p, s) I think that can be used instead of adding this TP_printk_init(). the reason add TP_printk_init() because when i first design some macro which not related to tracepoint, it use too much stack. Not sure what you mean about 'uses too much stack'. This is called by the reading code and not some arbitrary location, and the above macros are done in the same location as your "init" call, so I'm not sure how that makes a difference on the stack. but i think TP_printk_init() is good as it following most common way to print. I really do not want to add more versions of TRACE_EVENT() that I need to maintain unless there is a really good reason to do so. And I really don't want to encourage the use of a "TP_printk_init()" because that just encourages more use cases that will make it hard for user space to parse the TP_printk(). that's true, it is difficult to understand, when i add this new, it report build issue. will consider other way for this case without new tracepoint macro. -- Steve
Re: [PATCH 3/8] usb: udc: trace: reduce buffer usage of trace event
On Fri, 15 Sep 2023 09:11:06 +0800 Linyu Yuan wrote: > >> + snprintf(__s, 9, "ep%d%s", te.address, \ > >> + (te.caps.dir_in && te.caps.dir_out) ? "" : \ > >> + te.caps.dir_in ? "in" : "out"); > > Note, there's a temp buffer trace_seq 'p' available for use as well. See > > both include/trace/events/libata.h and include/trace/events/scsi.h: > > > >const char *libata_trace_parse_status(struct trace_seq*, unsigned char); > >#define __parse_status(s) libata_trace_parse_status(p, s) > > > > I think that can be used instead of adding this TP_printk_init(). > > > the reason add TP_printk_init() because when i first design some macro > which not > > related to tracepoint, it use too much stack. > Not sure what you mean about 'uses too much stack'. This is called by the reading code and not some arbitrary location, and the above macros are done in the same location as your "init" call, so I'm not sure how that makes a difference on the stack. > > but i think TP_printk_init() is good as it following most common way > to print. > I really do not want to add more versions of TRACE_EVENT() that I need to maintain unless there is a really good reason to do so. And I really don't want to encourage the use of a "TP_printk_init()" because that just encourages more use cases that will make it hard for user space to parse the TP_printk(). -- Steve
[ANNOUNCE] 4.14.325-rt154
Hello RT-list! I'm pleased to announce the 4.14.325-rt154 stable release. This release is just an update to the new stable 4.14.325 version and no RT specific changes have been made. You can get this release via the git tree at: git://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git branch: v4.14-rt Head SHA1: d68e56f9037b8642366fbb2f5f9b11e1a0636760 Or to build 4.14.325-rt154 directly, the following patches should be applied: https://www.kernel.org/pub/linux/kernel/v4.x/linux-4.14.tar.xz https://www.kernel.org/pub/linux/kernel/v4.x/patch-4.14.325.xz https://www.kernel.org/pub/linux/kernel/projects/rt/4.14/older/patch-4.14.325-rt154.patch.xz Signing key fingerprint: 9354 0649 9972 8D31 D464 D140 F394 A423 F8E6 7C26 All keys used for the above files and repositories can be found on the following git repository: git://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git Enjoy! Luis
[ANNOUNCE] 5.10.194-rt94
Hello RT-list! I'm pleased to announce the 5.10.194-rt94 stable release. This release is just an update to the new stable 5.10.194 version and no RT specific changes have been made. You can get this release via the git tree at: git://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git branch: v5.10-rt Head SHA1: 7d2f06611db33d0957999cc9354e5ac92a8db387 Or to build 5.10.194-rt94 directly, the following patches should be applied: https://www.kernel.org/pub/linux/kernel/v5.x/linux-5.10.tar.xz https://www.kernel.org/pub/linux/kernel/v5.x/patch-5.10.194.xz https://www.kernel.org/pub/linux/kernel/projects/rt/5.10/older/patch-5.10.194-rt94.patch.xz Signing key fingerprint: 9354 0649 9972 8D31 D464 D140 F394 A423 F8E6 7C26 All keys used for the above files and repositories can be found on the following git repository: git://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git Enjoy! Luis
Re: [PATCH 2/8] usb: gadget: add anonymous definition in some struct for trace purpose
On 9/15/2023 9:51 AM, Alan Stern wrote: On Fri, Sep 15, 2023 at 09:02:48AM +0800, Linyu Yuan wrote: On 9/14/2023 10:54 PM, Alan Stern wrote: You didn't include the version number in the Subject: line. Undoubtedly Greg's automatic error checker will warn you about this. Unless the version number is clearly marked for each patch, it's difficult for his programs to tell which email message contains the most recent version. On Thu, Sep 14, 2023 at 06:02:56PM +0800, Linyu Yuan wrote: Some UDC trace event will save usb udc information, but it use one int size buffer to save one bit information of usb udc, it is wast trace buffer. Add anonymous union which have one u32 member can be used by trace event during fast assign stage to save more entries with same trace ring buffer size. Signed-off-by: Linyu Yuan --- And you didn't include the version change information here, below the "---" line. Apart from that, this is a _lot_ better than before! I don't know if Greg will think this change is worth merging, but at least now it's possible to read the code and understand what's going on. according Steven's comment, maybe will always save data in little endian at trace event fast assign stage. it will add definition of bit field back. Yes, that would be even better because you wouldn't have to change the definition of struct usb_gadget or struct usb_endpoint at all. The fast assign stage can simply do: __entry->dw1 = (g->sg_supported << 0) | (g->is_otg << 1) | ... and then you can easily access the individual bits in __entry. It wouldn't be as fast but it would still save a lot of space. how about __entry->dw1 = cpu_to_le32(g->dw1) ? Alan Stern
Re: [PATCH 2/8] usb: gadget: add anonymous definition in some struct for trace purpose
On Fri, Sep 15, 2023 at 09:02:48AM +0800, Linyu Yuan wrote: > > On 9/14/2023 10:54 PM, Alan Stern wrote: > > You didn't include the version number in the Subject: line. Undoubtedly > > Greg's automatic error checker will warn you about this. Unless the > > version number is clearly marked for each patch, it's difficult for his > > programs to tell which email message contains the most recent version. > > > > On Thu, Sep 14, 2023 at 06:02:56PM +0800, Linyu Yuan wrote: > > > Some UDC trace event will save usb udc information, but it use one int > > > size buffer to save one bit information of usb udc, it is wast trace > > > buffer. > > > > > > Add anonymous union which have one u32 member can be used by trace event > > > during fast assign stage to save more entries with same trace ring buffer > > > size. > > > > > > Signed-off-by: Linyu Yuan > > > --- > > And you didn't include the version change information here, below the > > "---" line. > > > > Apart from that, this is a _lot_ better than before! I don't know if > > Greg will think this change is worth merging, but at least now it's > > possible to read the code and understand what's going on. > > > according Steven's comment, maybe will always save data in little endian at > trace event > > fast assign stage. > > it will add definition of bit field back. Yes, that would be even better because you wouldn't have to change the definition of struct usb_gadget or struct usb_endpoint at all. The fast assign stage can simply do: __entry->dw1 = (g->sg_supported << 0) | (g->is_otg << 1) | ... and then you can easily access the individual bits in __entry. It wouldn't be as fast but it would still save a lot of space. Alan Stern
Re: [PATCH 3/8] usb: udc: trace: reduce buffer usage of trace event
On 9/15/2023 12:54 AM, Steven Rostedt wrote:
On Thu, 14 Sep 2023 18:02:57 +0800
Linyu Yuan wrote:
Save u32 members into trace event ring buffer and parse it for possible
bit fields.
Use new DECLARE_EVENT_CLASS_PRINT_INIT() class macro for output stage.
Signed-off-by: Linyu Yuan
---
drivers/usb/gadget/udc/trace.h | 154 +++--
1 file changed, 69 insertions(+), 85 deletions(-)
diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
index a5ed26fbc2da..e1754667f1d2 100644
--- a/drivers/usb/gadget/udc/trace.h
+++ b/drivers/usb/gadget/udc/trace.h
@@ -17,7 +17,7 @@
#include
#include
-DECLARE_EVENT_CLASS(udc_log_gadget,
+DECLARE_EVENT_CLASS_PRINT_INIT(udc_log_gadget,
TP_PROTO(struct usb_gadget *g, int ret),
TP_ARGS(g, ret),
TP_STRUCT__entry(
@@ -25,20 +25,7 @@ DECLARE_EVENT_CLASS(udc_log_gadget,
__field(enum usb_device_speed, max_speed)
__field(enum usb_device_state, state)
__field(unsigned, mA)
- __field(unsigned, sg_supported)
- __field(unsigned, is_otg)
- __field(unsigned, is_a_peripheral)
- __field(unsigned, b_hnp_enable)
- __field(unsigned, a_hnp_support)
- __field(unsigned, hnp_polling_support)
- __field(unsigned, host_request_flag)
- __field(unsigned, quirk_ep_out_aligned_size)
- __field(unsigned, quirk_altset_not_supp)
- __field(unsigned, quirk_stall_not_supp)
- __field(unsigned, quirk_zlp_not_supp)
- __field(unsigned, is_selfpowered)
- __field(unsigned, deactivated)
- __field(unsigned, connected)
+ __field(u32, gdw1)
__field(int, ret)
),
TP_fast_assign(
@@ -46,39 +33,35 @@ DECLARE_EVENT_CLASS(udc_log_gadget,
__entry->max_speed = g->max_speed;
__entry->state = g->state;
__entry->mA = g->mA;
- __entry->sg_supported = g->sg_supported;
- __entry->is_otg = g->is_otg;
- __entry->is_a_peripheral = g->is_a_peripheral;
- __entry->b_hnp_enable = g->b_hnp_enable;
- __entry->a_hnp_support = g->a_hnp_support;
- __entry->hnp_polling_support = g->hnp_polling_support;
- __entry->host_request_flag = g->host_request_flag;
- __entry->quirk_ep_out_aligned_size =
g->quirk_ep_out_aligned_size;
- __entry->quirk_altset_not_supp = g->quirk_altset_not_supp;
- __entry->quirk_stall_not_supp = g->quirk_stall_not_supp;
- __entry->quirk_zlp_not_supp = g->quirk_zlp_not_supp;
- __entry->is_selfpowered = g->is_selfpowered;
- __entry->deactivated = g->deactivated;
- __entry->connected = g->connected;
+ __entry->gdw1 = g->dw1;
__entry->ret = ret;
),
- TP_printk("speed %d/%d state %d %dmA [%s%s%s%s%s%s%s%s%s%s%s%s%s%s] -->
%d",
+ TP_printk("speed %d/%d state %d %dmA [%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s]
--> %d",
__entry->speed, __entry->max_speed, __entry->state, __entry->mA,
- __entry->sg_supported ? "sg:" : "",
- __entry->is_otg ? "OTG:" : "",
- __entry->is_a_peripheral ? "a_peripheral:" : "",
- __entry->b_hnp_enable ? "b_hnp:" : "",
- __entry->a_hnp_support ? "a_hnp:" : "",
- __entry->hnp_polling_support ? "hnp_poll:" : "",
- __entry->host_request_flag ? "hostreq:" : "",
- __entry->quirk_ep_out_aligned_size ? "out_aligned:" : "",
- __entry->quirk_altset_not_supp ? "no_altset:" : "",
- __entry->quirk_stall_not_supp ? "no_stall:" : "",
- __entry->quirk_zlp_not_supp ? "no_zlp" : "",
- __entry->is_selfpowered ? "self-powered:" : "bus-powered:",
- __entry->deactivated ? "deactivated:" : "activated:",
- __entry->connected ? "connected" : "disconnected",
- __entry->ret)
+ tg.sg_supported ? "sg:" : "",
+ tg.is_otg ? "OTG:" : "",
+ tg.is_a_peripheral ? "a_peripheral:" : "",
+ tg.b_hnp_enable ? "b_hnp:" : "",
+ tg.a_hnp_support ? "a_hnp:" : "",
+ tg.a_alt_hnp_support ? "a_alt_hnp:" : "",
+ tg.hnp_polling_support ? "hnp_poll:" : "",
+ tg.host_request_flag ? "hostreq:" : "",
+ tg.quirk_ep_out_aligned_size ? "out_aligned:" : "",
+ tg.quirk_altset_not_supp ? "no_altset:" : "",
+ tg.quirk_stall_not_supp ? "no_stall:" : "",
+ tg.quirk_zlp_not_supp ? "no_zlp" : "",
+ tg.quirk_avoids_skb_reserve ? "no_skb_reserve" : "",
+ tg.is_selfpowered ? "self-powered:" :
Re: [PATCH 2/8] usb: gadget: add anonymous definition in some struct for trace purpose
On 9/14/2023 10:54 PM, Alan Stern wrote: You didn't include the version number in the Subject: line. Undoubtedly Greg's automatic error checker will warn you about this. Unless the version number is clearly marked for each patch, it's difficult for his programs to tell which email message contains the most recent version. On Thu, Sep 14, 2023 at 06:02:56PM +0800, Linyu Yuan wrote: Some UDC trace event will save usb udc information, but it use one int size buffer to save one bit information of usb udc, it is wast trace buffer. Add anonymous union which have one u32 member can be used by trace event during fast assign stage to save more entries with same trace ring buffer size. Signed-off-by: Linyu Yuan --- And you didn't include the version change information here, below the "---" line. Apart from that, this is a _lot_ better than before! I don't know if Greg will think this change is worth merging, but at least now it's possible to read the code and understand what's going on. according Steven's comment, maybe will always save data in little endian at trace event fast assign stage. it will add definition of bit field back. Alan Stern
Re: linux-next: Tree for Sep 12 (bcachefs)
On Thu, Sep 14, 2023 at 03:38:07PM -0400, Kent Overstreet wrote:
> On Wed, Sep 13, 2023 at 06:17:00PM -0700, Kees Cook wrote:
> > It looks like you just want a type union for the flexible array.
> > This can be done like this:
> >
> > struct bch_sb_field_journal_seq_blacklist {
> > struct bch_sb_field field;
> >
> > union {
> > DECLARE_FLEX_ARRAY(struct journal_seq_blacklist_entry, start);
> > DECLARE_FLEX_ARRAY(__u64, _data);
> > };
> > };
>
> Eesh, why though?
>
> Honestly, I'm not a fan of the change to get rid of zero size arrays,
> this seems to be adding a whole lot of macro layering and indirection
> for nothing.
The C standard doesn't help us in that regard, that's true. But we've
been working to get it fixed. For example, there's discussion happening
next week at GNU Cauldron about flexible arrays in unions. It's already
possible, so better to just fix the standard -- real world code needs it
and uses it, as the bcachefs code illustrates. :)
> The only thing a zero size array could possibly be is a flexible array
> member or a marker, why couldn't we have just kept treating zero size
> arrays like flexible array members?
Because they're ambiguous and then the compiler can't do appropriate
bounds checking, compile-time diagnostics, etc. Maybe it's actually zero
sized, maybe it's not. Nothing stops them from being in the middle of
the structure so if someone accidentally tries to put members after it
(which has happened before), we end up with bizarre corruptions, etc,
etc. Flexible arrays are unambiguous, and that's why we committed to
converting all the fake flex arrays. The compiler does not have to guess
(or as has been the case: give up on) figuring out what was intended.
Regardless, I'm just trying to help make sure folks that run with
CONFIG_UBSAN_BOUNDS=y (as done in Android, Ubuntu, etc) will be able to
use bcachefs without runtime warnings, etc. Indexing through a 0-sized
array is going to trip the diagnostic either at runtime or when building
with -Warray-bounds.
-Kees
--
Kees Cook
[PATCH] HID: uhid: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1].
We should prefer more robust and less ambiguous string interfaces.
A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.
Looking at: Commit 4d26d1d1e806 ("Revert "HID: uhid: use strlcpy() instead of
strncpy()"")
we see referenced the fact that many attempts have been made to change
these strncpy's into strlcpy to no success. I think strscpy is an
objectively better interface here as it doesn't unnecessarily NUL-pad
the destination buffer whilst allowing us to drop the `len = min(...)`
pattern as strscpy will implicitly limit the number of bytes copied by
the smaller of its dest and src arguments.
So while the existing code may not have a bug (i.e: overread problems)
we should still favor strscpy due to readability (plus a very slight
performance boost).
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-harden...@vger.kernel.org
Cc: Kees Cook
Signed-off-by: Justin Stitt
---
drivers/hid/uhid.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 4588d2cd4ea4..00e1566ad37b 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -490,7 +490,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
const struct uhid_event *ev)
{
struct hid_device *hid;
- size_t rd_size, len;
+ size_t rd_size;
void *rd_data;
int ret;
@@ -514,13 +514,9 @@ static int uhid_dev_create2(struct uhid_device *uhid,
goto err_free;
}
- /* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
- len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
- strncpy(hid->name, ev->u.create2.name, len);
- len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
- strncpy(hid->phys, ev->u.create2.phys, len);
- len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
- strncpy(hid->uniq, ev->u.create2.uniq, len);
+ strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
+ strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
+ strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));
hid->ll_driver = _hid_driver;
hid->bus = ev->u.create2.bus;
---
base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec
change-id: 20230914-strncpy-drivers-hid-uhid-c-a465f3e784dd
Best regards,
--
Justin Stitt
[PATCH] HID: prodikeys: refactor deprecated strncpy
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-harden...@vger.kernel.org Signed-off-by: Justin Stitt --- Note: for some reason if NUL-padding is needed let's opt for `strscpy_pad()` --- drivers/hid/hid-prodikeys.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c index e4e9471d0f1e..c16d2ba6ea16 100644 --- a/drivers/hid/hid-prodikeys.c +++ b/drivers/hid/hid-prodikeys.c @@ -639,9 +639,9 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) goto fail; } - strncpy(card->driver, shortname, sizeof(card->driver)); - strncpy(card->shortname, shortname, sizeof(card->shortname)); - strncpy(card->longname, longname, sizeof(card->longname)); + strscpy(card->driver, shortname, sizeof(card->driver)); + strscpy(card->shortname, shortname, sizeof(card->shortname)); + strscpy(card->longname, longname, sizeof(card->longname)); /* Set up rawmidi */ err = snd_rawmidi_new(card, card->shortname, 0, @@ -652,7 +652,7 @@ static int pcmidi_snd_initialise(struct pcmidi_snd *pm) goto fail; } pm->rwmidi = rwmidi; - strncpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); + strscpy(rwmidi->name, card->shortname, sizeof(rwmidi->name)); rwmidi->info_flags = SNDRV_RAWMIDI_INFO_INPUT; rwmidi->private_data = pm; --- base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec change-id: 20230914-strncpy-drivers-hid-hid-prodikeys-c-cf42614a21d4 Best regards, -- Justin Stitt
Re: [PATCH v4 03/18] x86/sgx: Add sgx_epc_lru_lists to encapsulate LRU lists
On Thu, 2023-09-14 at 09:13 -0700, Dave Hansen wrote: > On 9/14/23 03:31, Huang, Kai wrote: > > > Signed-off-by: Haitao Huang > > > Cc: Sean Christopherson > > You don't need 'Cc:' Sean if the patch has Sean's SoB. > > It is a SoB for Sean's @intel address and cc's his @google address. > > It is fine. Oops I didn't notice the email difference. Thanks for pointing out!
Re: [PATCH] nd_btt: Make BTT lanes preemptible
Tomáš Glozar wrote:
> From: Tomas Glozar
>
> nd_region_acquire_lane uses get_cpu, which disables preemption. This is
> an issue on PREEMPT_RT kernels, since btt_write_pg and also
> nd_region_acquire_lane itself take a spin lock, resulting in BUG:
> sleeping function called from invalid context.
Is the bug in 1 of 2 places?
1) When btt_write_pg()->lock_map() (when the number of lanes is < number
of cpus) and the lane is acquired is called?
*or*
2) When nd_region_acquire_lane() internally trys to take it's lock?
A copy/paste of the BUG observed would have been more clear I think.
Regardless I *think* this is ok but I'm worried I don't fully understand
what the problem is.
Ira
>
> Fix the issue by replacing get_cpu with smp_process_id and
> migrate_disable when needed. This makes BTT operations preemptible, thus
> permitting the use of spin_lock.
>
> Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
> Signed-off-by: Tomas Glozar
> ---
> drivers/nvdimm/region_devs.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index 0a81f87f6f6c..e2f1fb99707f 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -939,7 +939,8 @@ unsigned int nd_region_acquire_lane(struct nd_region
> *nd_region)
> {
> unsigned int cpu, lane;
>
> - cpu = get_cpu();
> + migrate_disable();
> + cpu = smp_processor_id();
> if (nd_region->num_lanes < nr_cpu_ids) {
> struct nd_percpu_lane *ndl_lock, *ndl_count;
>
> @@ -958,16 +959,15 @@ EXPORT_SYMBOL(nd_region_acquire_lane);
> void nd_region_release_lane(struct nd_region *nd_region, unsigned int lane)
> {
> if (nd_region->num_lanes < nr_cpu_ids) {
> - unsigned int cpu = get_cpu();
> + unsigned int cpu = smp_processor_id();
> struct nd_percpu_lane *ndl_lock, *ndl_count;
>
> ndl_count = per_cpu_ptr(nd_region->lane, cpu);
> ndl_lock = per_cpu_ptr(nd_region->lane, lane);
> if (--ndl_count->count == 0)
> spin_unlock(_lock->lock);
> - put_cpu();
> }
> - put_cpu();
> + migrate_enable();
> }
> EXPORT_SYMBOL(nd_region_release_lane);
>
> --
> 2.39.3
>
Re: linux-next: Tree for Sep 12 (bcachefs)
On 9/14/23 13:38, Kent Overstreet wrote:
On Wed, Sep 13, 2023 at 06:17:00PM -0700, Kees Cook wrote:
On Tue, Sep 12, 2023 at 03:26:45PM +1000, Stephen Rothwell wrote:
New tree: bcachefs
Thanks for going through and fixing all the fake flexible array members.
It looks much nicer. :)
I have some questions about the remaining "markers", for example:
$ git grep -A8 '\bkey_start\b' -- fs/bcachefs
fs/bcachefs/bcachefs_format.h: __u8key_start[0];
...
fs/bcachefs/bcachefs_format.h- __u8pad[sizeof(struct bkey) - 3];
--
fs/bcachefs/bkey.c: u8 *l = k->key_start;
Why isn't this just:
u8 *l = k->pad
and you can drop the marker?
In this case, it's documentation. >pad tells us nothing; why is pad
significant? k->key_start documents the intent better.
And some seem entirely unused, like all of "struct bch_reflink_v".
No, those aren't unused :)
bcachefs does the "list of variable size items" a lot - see vstructs.h.
start[] is the type of the item being stored, _data is what we use for
pointer arithmetic - because we always store sizes in units of u64s, for
alignment.
And some are going to fail at runtime, since they're still zero-sized
and being used as an actual array:
struct bch_sb_field_journal_seq_blacklist {
struct bch_sb_field field;
struct journal_seq_blacklist_entry start[0];
__u64 _data[];
};
...
memmove(>start[i],
>start[i + 1],
sizeof(bl->start[0]) * (nr - i));
It looks like you just want a type union for the flexible array.
This can be done like this:
struct bch_sb_field_journal_seq_blacklist {
struct bch_sb_field field;
union {
DECLARE_FLEX_ARRAY(struct journal_seq_blacklist_entry, start);
DECLARE_FLEX_ARRAY(__u64, _data);
};
};
Eesh, why though?
Honestly, I'm not a fan of the change to get rid of zero size arrays,
this seems to be adding a whole lot of macro layering and indirection
for nothing.
The only thing a zero size array could possibly be is a flexible array
member or a marker, why couldn't we have just kept treating zero size
arrays like flexible array members?
Because zero-length arrays, when used as fake flexible arrays, make
things like -Warray-bounds (we've been trying to enable this compiler
option, globally) trip; among other things like being prone to result in
undefined behavior bugs when people introduce new members that make the
array end up in the middle of its containing structure.
With C99 flexible-array members, the compiler emits a warning when the
arrays are not at the end of the structure.
The DECLARE_FLEX_ARRAY() (in a union) helper allows for multiple C99
flexible-array members together at the end of a struct.
--
Gustavo
Re: linux-next: Tree for Sep 12 (bcachefs)
On Wed, Sep 13, 2023 at 06:17:00PM -0700, Kees Cook wrote:
> On Tue, Sep 12, 2023 at 03:26:45PM +1000, Stephen Rothwell wrote:
> > New tree: bcachefs
>
> Thanks for going through and fixing all the fake flexible array members.
> It looks much nicer. :)
>
> I have some questions about the remaining "markers", for example:
>
> $ git grep -A8 '\bkey_start\b' -- fs/bcachefs
> fs/bcachefs/bcachefs_format.h: __u8key_start[0];
> ...
> fs/bcachefs/bcachefs_format.h- __u8pad[sizeof(struct bkey) - 3];
> --
> fs/bcachefs/bkey.c: u8 *l = k->key_start;
>
> Why isn't this just:
>
> u8 *l = k->pad
>
> and you can drop the marker?
In this case, it's documentation. >pad tells us nothing; why is pad
significant? k->key_start documents the intent better.
> And some seem entirely unused, like all of "struct bch_reflink_v".
No, those aren't unused :)
bcachefs does the "list of variable size items" a lot - see vstructs.h.
start[] is the type of the item being stored, _data is what we use for
pointer arithmetic - because we always store sizes in units of u64s, for
alignment.
>
> And some are going to fail at runtime, since they're still zero-sized
> and being used as an actual array:
>
> struct bch_sb_field_journal_seq_blacklist {
> struct bch_sb_field field;
>
> struct journal_seq_blacklist_entry start[0];
> __u64 _data[];
> };
> ...
> memmove(>start[i],
> >start[i + 1],
> sizeof(bl->start[0]) * (nr - i));
>
> It looks like you just want a type union for the flexible array.
> This can be done like this:
>
> struct bch_sb_field_journal_seq_blacklist {
> struct bch_sb_field field;
>
> union {
> DECLARE_FLEX_ARRAY(struct journal_seq_blacklist_entry, start);
> DECLARE_FLEX_ARRAY(__u64, _data);
> };
> };
Eesh, why though?
Honestly, I'm not a fan of the change to get rid of zero size arrays,
this seems to be adding a whole lot of macro layering and indirection
for nothing.
The only thing a zero size array could possibly be is a flexible array
member or a marker, why couldn't we have just kept treating zero size
arrays like flexible array members?
Re: [PATCH v3] libnvdimm/of_pmem: Use devm_kstrdup instead of kstrdup and check its return value
Dave Jiang wrote:
>
>
> On 9/14/23 00:03, Chen Ni wrote:
[snip]
> > diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> > index 1b9f5b8a6167..5765674b36f2 100644
> > --- a/drivers/nvdimm/of_pmem.c
> > +++ b/drivers/nvdimm/of_pmem.c
> > @@ -30,7 +30,13 @@ static int of_pmem_region_probe(struct platform_device
> > *pdev)
> > if (!priv)
> > return -ENOMEM;
> >
> > - priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> > + priv->bus_desc.provider_name = devm_kstrdup(>dev, pdev->name,
> > + GFP_KERNEL);
> > + if (!priv->bus_desc.provider_name) {
> > + kfree(priv);
>
> I wonder if priv should be allocated with devm_kzalloc() instead to reduce
> the resource management burden.
I think it could be but this is the driver and I wonder if leaving the
allocation around until the platform device goes away was undesirable for
some reason?
Ira
Re: [PATCH] module: print module name on refcount error
On Mon 28-08-23 14:18:30, Jean Delvare wrote: [...] > > > It would likely be better to use refcount_t instead of atomic_t. > > > > Patches welcomed. > > Michal, do I understand correctly that this would prevent the case our > customer had (too many gets), but won't make a difference for actual > too-many-puts situations? yes, refcount_t cannot protect from too-many-puts because there is not real way to protect from those AFAICS. At a certain moment you just drop to 0 and lose your object and all following that is a UAF. But I do not think this is actually the interesting case at all. -- Michal Hocko SUSE Labs
Re: [PATCH 03/19] fs: release anon dev_t in deactivate_locked_super
On Thu, Sep 14, 2023 at 05:58:05PM +0100, Al Viro wrote: > Incidentally, I'm going to add a (belated by 10 years) chunk in porting.rst > re making sure that anything in superblock that might be needed by methods > called in RCU mode should *not* be freed without an RCU delay... Should've > done that back in 3.12 merge window when RCU'd vfsmounts went in; as it > is, today we have several filesystems with exact same kind of breakage. > hfsplus and affs breakage had been there in 3.13 (missed those two), exfat > and ntfs3 - introduced later, by initial merges of filesystems in question. > Missed on review... > > Hell knows - perhaps Documentation/filesystems/whack-a-mole might be a good > idea... Actually, utf8 casefolding stuff also has the same problem, so ext4 and f2fs with casefolding are also affected ;-/
Re: [PATCH v1 1/1] lib/string_helpers: Don't copy a tail in kstrdup_and_replace() if 'new' is \0
On Wed, Sep 13, 2023 at 12:45:57PM +0300, Andy Shevchenko wrote: > The kstrdup_and_replace() takes two characters, old and new, to replace > former with latter after the copying of the original string. But in case > when new is a NUL, there is no point to copy the rest of the string, > the contract with the callers is that that the function returns a > NUL-terminated string and not a buffer of the size filled with a given > data. With this we can optimize the memory consumption by copying only > meaningful part of the original string and drop the rest. Thinking about this more, I self NAK this. If the caller knows the size of the original message it can be handy to make a copy and replace all occurrences of old by NUL. This will be an optimized implementation of strsep(str, "$OLD"). -- With Best Regards, Andy Shevchenko
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On 14/09/2023 18:42, Beau Belgrave wrote:
> On Thu, Sep 14, 2023 at 03:11:02PM +0200, Clément Léger wrote:
>> enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
>> this will result in a misaligned access on 64 bits architectures since
>> set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
>> On architecture that do not support misaligned access, this will crash
>> the kernel. Align uaddr on unsigned long size to avoid such behavior.
>> This bug was found while running kselftests on RISC-V.
>>
>> Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event
>> enablement")
>> Signed-off-by: Clément Léger
>
> Thanks for fixing! I have a few comments on this.
>
> I unfortunately do not have RISC-V hardware to validate this on.
>
>> ---
>> kernel/trace/trace_events_user.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events_user.c
>> b/kernel/trace/trace_events_user.c
>> index 6f046650e527..580c0fe4b23e 100644
>> --- a/kernel/trace/trace_events_user.c
>> +++ b/kernel/trace/trace_events_user.c
>> @@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm
>> *mm,
>> bool fixup_fault, int *attempt)
>> {
>> unsigned long uaddr = enabler->addr;
>> -unsigned long *ptr;
>> +unsigned long *ptr, bit_offset;
>> struct page *page;
>> void *kaddr;
>> int ret;
>> @@ -511,13 +511,19 @@ static int user_event_enabler_write(struct
>> user_event_mm *mm,
>> }
>>
>> kaddr = kmap_local_page(page);
>> +
>> +bit_offset = uaddr & (sizeof(unsigned long) - 1);
>> +if (bit_offset) {
>> +bit_offset *= 8;
>
> I think for future readers of this code it would be more clear to use
> BITS_PER_BYTE instead of the hardcoded 8. Given we always align on a
> "natural" boundary, I believe the bit_offset will always be 32 bits.
>
> A comment here might help clarify why we do this as well in case folks
> don't see the change description.
Hi Beau,
Yes sure, I'll add a comment and use the define as well.
>
>> +uaddr &= ~(sizeof(unsigned long) - 1);
>
> Shouldn't this uaddr change be done before calling pin_user_pages_remote()
> to ensure things cannot go bad? (I don't think they can, but it looks a
> little odd).
Indeed, I don't think that will cause any problem since pin_user_pages
will return a page aligned address anyway and that aligning uaddr will
not yield any page crossing. But I'll check to be sure and move that
before the call if needed.
Clément
>
> Thanks,
> -Beau
>
>> +}
>> ptr = kaddr + (uaddr & ~PAGE_MASK);
>>
>> /* Update bit atomically, user tracers must be atomic as well */
>> if (enabler->event && enabler->event->status)
>> -set_bit(ENABLE_BIT(enabler), ptr);
>> +set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
>> else
>> -clear_bit(ENABLE_BIT(enabler), ptr);
>> +clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
>>
>> kunmap_local(kaddr);
>> unpin_user_pages_dirty_lock(, 1, true);
>> --
>> 2.40.1
Re: [PATCH RFC 00/37] Add support for arm64 MTE dynamic tag storage reuse
Hi Kuan-Ying,
On Wed, Sep 13, 2023 at 08:11:40AM +, Kuan-Ying Lee (李冠穎) wrote:
> On Wed, 2023-08-23 at 14:13 +0100, Alexandru Elisei wrote:
> > diff --git a/arch/arm64/boot/dts/arm/fvp-base-revc.dts
> > b/arch/arm64/boot/dts/arm/fvp-base-revc.dts
> > index 60472d65a355..bd050373d6cf 100644
> > --- a/arch/arm64/boot/dts/arm/fvp-base-revc.dts
> > +++ b/arch/arm64/boot/dts/arm/fvp-base-revc.dts
> > @@ -165,10 +165,28 @@ C1_L2: l2-cache1 {
> > };
> > };
> >
> > - memory@8000 {
> > + memory0: memory@8000 {
> > device_type = "memory";
> > - reg = <0x 0x8000 0 0x8000>,
> > - <0x0008 0x8000 0 0x8000>;
> > + reg = <0x00 0x8000 0x00 0x7c00>;
> > + };
> > +
> > + metadata0: metadata@c000 {
> > + compatible = "arm,mte-tag-storage";
> > + reg = <0x00 0xfc00 0x00 0x3e0>;
> > + block-size = <0x1000>;
> > + memory = <>;
> > + };
> > +
> > + memory1: memory@88000 {
> > + device_type = "memory";
> > + reg = <0x08 0x8000 0x00 0x7c00>;
> > + };
> > +
> > + metadata1: metadata@8c000 {
> > + compatible = "arm,mte-tag-storage";
> > + reg = <0x08 0xfc00 0x00 0x3e0>;
> > + block-size = <0x1000>;
> > + memory = <>;
> > };
> >
>
> AFAIK, the above memory configuration means that there are two region
> of dram(0x8000-0xfc00 and 0x8_8000-0x8_fc000) and this
> is called PDD memory map.
>
> Document[1] said there are some constraints of tag memory as below.
>
> | The following constraints apply to the tag regions in DRAM:
> | 1. The tag region cannot be interleaved with the data region.
> | The tag region must also be above the data region within DRAM.
> |
> | 2.The tag region in the physical address space cannot straddle
> | multiple regions of a memory map.
> |
> | PDD memory map is not allowed to have part of the tag region between
> | 2GB-4GB and another part between 34GB-64GB.
>
> I'm not sure if we can separate tag memory with the above
> configuration. Or do I miss something?
>
> [1] https://developer.arm.com/documentation/101569/0300/?lang=en
> (Section 5.4.6.1)
Good point, thanks. The above dts some random layout we picked as an
example, it doesn't match any real hardware and we didn't pay attention
to the interconnect limitations (we fake the tag storage on the model).
I'll try to dig out how the mtu_tag_addr_shutter registers work and how
the sparse DRAM space is compressed to a smaller tag range. But that's
something done by firmware and the kernel only learns the tag storage
location from the DT (provided by firmware). We also don't need to know
the fine-grained mapping between 32 bytes of data and 1 byte (2 tags) in
the tag storage, only the block size in the tag storage space that
covers all interleaving done by the interconnect (it can be from 1 byte
to something larger like a page; the kernel will then use the lowest
common multiple between a page size and this tag block size to figure
out how many pages to reserve).
--
Catalin
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On 14/09/2023 19:29, Steven Rostedt wrote:
> On Thu, 14 Sep 2023 13:17:00 -0400
> Steven Rostedt wrote:
>
>> Now lets look at big endian layout:
>>
>> uaddr = 0xbeef0004
>> enabler = 1;
>>
>> memory at 0xbeef: 00 00 00 00 00 00 00 02
>> ^
>> addr: 0xbeef0004
>>
>> (enabler is set )
>>
>> bitoffset = uaddr & (sizeof(unsigned long) - 1); bitoffset = 4
>> bit_offset *= 8; bitoffset = 32
>> uaddr &= ~(sizeof(unsigned long) - 1); uaddr = 0xbeef
>>
>> ptr = kaddr + (uaddr & ~PAGE_MASK);
>>
>> clear_bit(1 + 32, ptr);
>>
>> memory at 0xbeef: 00 00 00 00 00 00 00 02
>> ^
>> bit 33 of 0xbeef
>>
>> I don't think that's what you expected!
>
> I believe the above can be fixed with:
>
> bit_offset = uaddr & (sizeof(unsigned long) - 1);
> if (bit_offset) {
> #ifdef CONFIG_CPU_BIG_ENDIAN
> bit_offest = 0;
> #else
> bit_offset *= BITS_PER_BYTE;
> #endif
> uaddr &= ~(sizeof(unsigned long) - 1);
> }
Hi Steven,
Nice catch ! I don't have big endian at end but I'll check that.
Thanks,
>
> -- Steve
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On Thu, 14 Sep 2023 13:17:00 -0400
Steven Rostedt wrote:
> Now lets look at big endian layout:
>
> uaddr = 0xbeef0004
> enabler = 1;
>
> memory at 0xbeef: 00 00 00 00 00 00 00 02
> ^
> addr: 0xbeef0004
>
> (enabler is set )
>
> bitoffset = uaddr & (sizeof(unsigned long) - 1); bitoffset = 4
> bit_offset *= 8; bitoffset = 32
> uaddr &= ~(sizeof(unsigned long) - 1); uaddr = 0xbeef
>
> ptr = kaddr + (uaddr & ~PAGE_MASK);
>
> clear_bit(1 + 32, ptr);
>
> memory at 0xbeef: 00 00 00 00 00 00 00 02
> ^
> bit 33 of 0xbeef
>
> I don't think that's what you expected!
I believe the above can be fixed with:
bit_offset = uaddr & (sizeof(unsigned long) - 1);
if (bit_offset) {
#ifdef CONFIG_CPU_BIG_ENDIAN
bit_offest = 0;
#else
bit_offset *= BITS_PER_BYTE;
#endif
uaddr &= ~(sizeof(unsigned long) - 1);
}
-- Steve
Re: [PATCH v2] wifi: brcmfmac: Replace 1-element arrays with flexible arrays
On 9/14/23 01:02, Juerg Haefliger wrote:
Since commit 2d47c6956ab3 ("ubsan: Tighten UBSAN_BOUNDS on GCC"),
UBSAN_BOUNDS no longer pretends 1-element arrays are unbounded. Walking
'element' and 'channel_list' will trigger warnings, so make them proper
flexible arrays.
False positive warnings were:
UBSAN: array-index-out-of-bounds in
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:6984:20
index 1 is out of range for type '__le32 [1]'
UBSAN: array-index-out-of-bounds in
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1126:27
index 1 is out of range for type '__le16 [1]'
for these lines of code:
6884 ch.chspec = (u16)le32_to_cpu(list->element[i]);
1126 params_le->channel_list[i] = cpu_to_le16(chanspec);
Cc: sta...@vger.kernel.org # 6.5+
Signed-off-by: Juerg Haefliger
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
---
v2:
- Use element[] instead of DFA() in brcmf_chanspec_list.
- Add Cc: stable tag
---
.../wireless/broadcom/brcm80211/brcmfmac/fwil_types.h| 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
index bece26741d3a..611d1a6aabb9 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
@@ -442,7 +442,12 @@ struct brcmf_scan_params_v2_le {
* fixed parameter portion is assumed, otherwise
* ssid in the fixed portion is ignored
*/
- __le16 channel_list[1]; /* list of chanspecs */
+ union {
+ __le16 padding; /* Reserve space for at least 1 entry for abort
+* which uses an on stack
brcmf_scan_params_v2_le
+*/
+ DECLARE_FLEX_ARRAY(__le16, channel_list); /* chanspecs */
+ };
};
struct brcmf_scan_results {
@@ -702,7 +707,7 @@ struct brcmf_sta_info_le {
struct brcmf_chanspec_list {
__le32 count; /* # of entries */
- __le32 element[1]; /* variable length uint32 list */
+ __le32 element[]; /* variable length uint32 list */
};
/*
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On Thu, 14 Sep 2023 15:11:02 +0200
Clément Léger wrote:
> @@ -511,13 +511,19 @@ static int user_event_enabler_write(struct
> user_event_mm *mm,
> }
>
> kaddr = kmap_local_page(page);
> +
> + bit_offset = uaddr & (sizeof(unsigned long) - 1);
> + if (bit_offset) {
> + bit_offset *= 8;
> + uaddr &= ~(sizeof(unsigned long) - 1);
> + }
> ptr = kaddr + (uaddr & ~PAGE_MASK);
>
> /* Update bit atomically, user tracers must be atomic as well */
> if (enabler->event && enabler->event->status)
> - set_bit(ENABLE_BIT(enabler), ptr);
> + set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
> else
> - clear_bit(ENABLE_BIT(enabler), ptr);
> + clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
>
> kunmap_local(kaddr);
> unpin_user_pages_dirty_lock(, 1, true);
Does this work for big endian machines too?
Little endian layout:
uaddr = 0xbeef0004
enabler = 1;
memory at 0xbeef: 00 00 00 00 02 00 00 00
^
addr: 0xbeef0004
(enabler is set )
bitoffset = uaddr & (sizeof(unsigned long) - 1); bitoffset = 4
bit_offset *= 8; bitoffset = 32
uaddr &= ~(sizeof(unsigned long) - 1); uaddr = 0xbeef
ptr = kaddr + (uaddr & ~PAGE_MASK);
clear_bit(1 + 32, ptr);
memory at 0xbeef: 00 00 00 00 02 00 00 00
^
bit 33 of 0xbeef
Now lets look at big endian layout:
uaddr = 0xbeef0004
enabler = 1;
memory at 0xbeef: 00 00 00 00 00 00 00 02
^
addr: 0xbeef0004
(enabler is set )
bitoffset = uaddr & (sizeof(unsigned long) - 1); bitoffset = 4
bit_offset *= 8; bitoffset = 32
uaddr &= ~(sizeof(unsigned long) - 1); uaddr = 0xbeef
ptr = kaddr + (uaddr & ~PAGE_MASK);
clear_bit(1 + 32, ptr);
memory at 0xbeef: 00 00 00 00 00 00 00 02
^
bit 33 of 0xbeef
I don't think that's what you expected!
-- Steve
Re: [PATCH 03/19] fs: release anon dev_t in deactivate_locked_super
On Thu, Sep 14, 2023 at 04:02:25PM +0200, Christian Brauner wrote: > Yes, you're right that making the superblock and not the filesytem type > the bd_holder changes the logic and we are aware of that of course. And > it requires changes such as moving additional block device closing from > where some callers currently do it. Details, please? > But the filesytem type is not a very useful holder itself and has other > drawbacks. The obvious one being that it requires us to wade through all > superblocks on the system trying to find the superblock associated with > a given block device continously grabbing and dropping sb_lock and > s_umount. None of that is very pleasant nor elegant and it is for sure > not very easy to understand (Plus, it's broken for btrfs freezing and > syncing via block level ioctls.). "Constantly" is a bit of a stretch - IIRC, we grabbed sb_lock once, then went through the list comparing ->s_bdev (without any extra locking), then bumped ->s_count on the found superblock, dropped sb_lock, grabbed ->s_umount on the sucker and verified it's still alive. Repeated grabbing of any lock happened only on a race with fs shutdown; normal case is one spin_lock, one spin_unlock, one down_read(). Oh, well... > Using the superblock as holder makes this go away and is overall a lot > more useful and intuitive and can be extended to filesystems with > multiple devices (Of which we apparently are bound to get more.). > > So I think this change is worth the pain. > > It's a fair point that these lifetime rules should be documented in > Documentation/filesystems/. The old lifetime documentation is too sparse > to be useful though. What *are* these lifetime rules? Seriously, you have 3 chunks of fs-dependent actions at the moment: * the things needed to get rid of internal references pinning inodes/dentries + whatever else we need done before generic_shutdown_super() * the stuff to be done between generic_shutdown_super() and making the sucker invisible to sget()/sget_fc() * the stuff that must be done after we are sure that sget callbacks won't be looking at this instance. Note that Christoph's series has mashed (2) and (3) together, resulting in UAF in a bunch of places. And I'm dead serious about Documentation/filesystems/porting being the right place; any development tree of any filesystem (in-tree one or not) will have to go through the changes and figure out WTF to do with their existing code. We are going to play whack-a-mole for at least several years as development branches get rebased and merged. Incidentally, I'm going to add a (belated by 10 years) chunk in porting.rst re making sure that anything in superblock that might be needed by methods called in RCU mode should *not* be freed without an RCU delay... Should've done that back in 3.12 merge window when RCU'd vfsmounts went in; as it is, today we have several filesystems with exact same kind of breakage. hfsplus and affs breakage had been there in 3.13 (missed those two), exfat and ntfs3 - introduced later, by initial merges of filesystems in question. Missed on review... Hell knows - perhaps Documentation/filesystems/whack-a-mole might be a good idea...
Re: [PATCH 3/8] usb: udc: trace: reduce buffer usage of trace event
On Thu, 14 Sep 2023 18:02:57 +0800
Linyu Yuan wrote:
> Save u32 members into trace event ring buffer and parse it for possible
> bit fields.
>
> Use new DECLARE_EVENT_CLASS_PRINT_INIT() class macro for output stage.
>
> Signed-off-by: Linyu Yuan
> ---
> drivers/usb/gadget/udc/trace.h | 154 +++--
> 1 file changed, 69 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
> index a5ed26fbc2da..e1754667f1d2 100644
> --- a/drivers/usb/gadget/udc/trace.h
> +++ b/drivers/usb/gadget/udc/trace.h
> @@ -17,7 +17,7 @@
> #include
> #include
>
> -DECLARE_EVENT_CLASS(udc_log_gadget,
> +DECLARE_EVENT_CLASS_PRINT_INIT(udc_log_gadget,
> TP_PROTO(struct usb_gadget *g, int ret),
> TP_ARGS(g, ret),
> TP_STRUCT__entry(
> @@ -25,20 +25,7 @@ DECLARE_EVENT_CLASS(udc_log_gadget,
> __field(enum usb_device_speed, max_speed)
> __field(enum usb_device_state, state)
> __field(unsigned, mA)
> - __field(unsigned, sg_supported)
> - __field(unsigned, is_otg)
> - __field(unsigned, is_a_peripheral)
> - __field(unsigned, b_hnp_enable)
> - __field(unsigned, a_hnp_support)
> - __field(unsigned, hnp_polling_support)
> - __field(unsigned, host_request_flag)
> - __field(unsigned, quirk_ep_out_aligned_size)
> - __field(unsigned, quirk_altset_not_supp)
> - __field(unsigned, quirk_stall_not_supp)
> - __field(unsigned, quirk_zlp_not_supp)
> - __field(unsigned, is_selfpowered)
> - __field(unsigned, deactivated)
> - __field(unsigned, connected)
> + __field(u32, gdw1)
> __field(int, ret)
> ),
> TP_fast_assign(
> @@ -46,39 +33,35 @@ DECLARE_EVENT_CLASS(udc_log_gadget,
> __entry->max_speed = g->max_speed;
> __entry->state = g->state;
> __entry->mA = g->mA;
> - __entry->sg_supported = g->sg_supported;
> - __entry->is_otg = g->is_otg;
> - __entry->is_a_peripheral = g->is_a_peripheral;
> - __entry->b_hnp_enable = g->b_hnp_enable;
> - __entry->a_hnp_support = g->a_hnp_support;
> - __entry->hnp_polling_support = g->hnp_polling_support;
> - __entry->host_request_flag = g->host_request_flag;
> - __entry->quirk_ep_out_aligned_size =
> g->quirk_ep_out_aligned_size;
> - __entry->quirk_altset_not_supp = g->quirk_altset_not_supp;
> - __entry->quirk_stall_not_supp = g->quirk_stall_not_supp;
> - __entry->quirk_zlp_not_supp = g->quirk_zlp_not_supp;
> - __entry->is_selfpowered = g->is_selfpowered;
> - __entry->deactivated = g->deactivated;
> - __entry->connected = g->connected;
> + __entry->gdw1 = g->dw1;
> __entry->ret = ret;
> ),
> - TP_printk("speed %d/%d state %d %dmA [%s%s%s%s%s%s%s%s%s%s%s%s%s%s] -->
> %d",
> + TP_printk("speed %d/%d state %d %dmA
> [%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s] --> %d",
> __entry->speed, __entry->max_speed, __entry->state, __entry->mA,
> - __entry->sg_supported ? "sg:" : "",
> - __entry->is_otg ? "OTG:" : "",
> - __entry->is_a_peripheral ? "a_peripheral:" : "",
> - __entry->b_hnp_enable ? "b_hnp:" : "",
> - __entry->a_hnp_support ? "a_hnp:" : "",
> - __entry->hnp_polling_support ? "hnp_poll:" : "",
> - __entry->host_request_flag ? "hostreq:" : "",
> - __entry->quirk_ep_out_aligned_size ? "out_aligned:" : "",
> - __entry->quirk_altset_not_supp ? "no_altset:" : "",
> - __entry->quirk_stall_not_supp ? "no_stall:" : "",
> - __entry->quirk_zlp_not_supp ? "no_zlp" : "",
> - __entry->is_selfpowered ? "self-powered:" : "bus-powered:",
> - __entry->deactivated ? "deactivated:" : "activated:",
> - __entry->connected ? "connected" : "disconnected",
> - __entry->ret)
> + tg.sg_supported ? "sg:" : "",
> + tg.is_otg ? "OTG:" : "",
> + tg.is_a_peripheral ? "a_peripheral:" : "",
> + tg.b_hnp_enable ? "b_hnp:" : "",
> + tg.a_hnp_support ? "a_hnp:" : "",
> + tg.a_alt_hnp_support ? "a_alt_hnp:" : "",
> + tg.hnp_polling_support ? "hnp_poll:" : "",
> + tg.host_request_flag ? "hostreq:" : "",
> + tg.quirk_ep_out_aligned_size ? "out_aligned:" : "",
> + tg.quirk_altset_not_supp ? "no_altset:" : "",
> + tg.quirk_stall_not_supp ? "no_stall:" : "",
> + tg.quirk_zlp_not_supp ? "no_zlp" : "",
> + tg.quirk_avoids_skb_reserve ? "no_skb_reserve" : "",
> + tg.is_selfpowered ? "self-powered:" :
Re: [PATCH 0/8] usb: gadget: reduce usb gadget trace event buffer usage
On Thu, 14 Sep 2023 18:02:54 +0800 Linyu Yuan wrote: > some trace event use an interger to to save a bit field info of gadget, > also some trace save endpoint name in string forat, it all can be > chagned to other way at trace event store phase. > > bit field can be replace with a union interger member which include > multiple bit fields. > > ep name stringe can be replace to a interger which contaion number > and dir info. > > to allow trace output stage can get bit info from save interger, > add DECLARE_EVENT_CLASS_PRINT_INIT() clas which allow user defined > operation before print. > > v1: > https://lore.kernel.org/linux-usb/20230911042843.2711-1-quic_linyy...@quicinc.com/ > v2: fix two compile issues that COMPILE_TEST not covered > > https://lore.kernel.org/linux-usb/2023092446.1791-1-quic_linyy...@quicinc.com/ > v3: fix reviewer comments, allow bit fields work on both little and big endian > > https://lore.kernel.org/linux-usb/20230912104455.7737-1-quic_linyy...@quicinc.com/ > v4: add DECLARE_EVENT_CLASS_PRINT_INIT() new trace class and use it > All these changes make it useless for user space. :-( -- Steve > Linyu Yuan (8): > trace: add new DECLARE_EVENT_CLASS_PRINT_INIT class type > usb: gadget: add anonymous definition in some struct for trace purpose > usb: udc: trace: reduce buffer usage of trace event > usb: cdns3: trace: reduce buffer usage of trace event > usb: dwc3: trace: reduce buffer usage of trace event > usb: cdns2: trace: reduce buffer usage of trace event > usb: mtu3: trace: reduce buffer usage of trace event > usb: musb: trace: reduce buffer usage of trace event > > drivers/usb/cdns3/cdns3-trace.h| 201 ++--- > drivers/usb/cdns3/cdnsp-trace.h| 105 +++ > drivers/usb/dwc3/trace.h | 99 ++ > drivers/usb/gadget/udc/cdns2/cdns2-trace.h | 175 -- > drivers/usb/gadget/udc/trace.h | 154 +++- > drivers/usb/mtu3/mtu3_trace.h | 76 +--- > drivers/usb/musb/musb_trace.h | 20 +- > include/linux/tracepoint.h | 22 +++ > include/linux/usb/gadget.h | 113 +++- > include/trace/bpf_probe.h | 4 + > include/trace/perf.h | 43 + > include/trace/stages/stage3_trace_output.h | 3 + > include/trace/trace_events.h | 118 > 13 files changed, 784 insertions(+), 349 deletions(-) >
Re: [PATCH] tracing/user_events: align uaddr on unsigned long alignment
On Thu, Sep 14, 2023 at 03:11:02PM +0200, Clément Léger wrote:
> enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
> this will result in a misaligned access on 64 bits architectures since
> set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
> On architecture that do not support misaligned access, this will crash
> the kernel. Align uaddr on unsigned long size to avoid such behavior.
> This bug was found while running kselftests on RISC-V.
>
> Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event
> enablement")
> Signed-off-by: Clément Léger
Thanks for fixing! I have a few comments on this.
I unfortunately do not have RISC-V hardware to validate this on.
> ---
> kernel/trace/trace_events_user.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_events_user.c
> b/kernel/trace/trace_events_user.c
> index 6f046650e527..580c0fe4b23e 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm
> *mm,
> bool fixup_fault, int *attempt)
> {
> unsigned long uaddr = enabler->addr;
> - unsigned long *ptr;
> + unsigned long *ptr, bit_offset;
> struct page *page;
> void *kaddr;
> int ret;
> @@ -511,13 +511,19 @@ static int user_event_enabler_write(struct
> user_event_mm *mm,
> }
>
> kaddr = kmap_local_page(page);
> +
> + bit_offset = uaddr & (sizeof(unsigned long) - 1);
> + if (bit_offset) {
> + bit_offset *= 8;
I think for future readers of this code it would be more clear to use
BITS_PER_BYTE instead of the hardcoded 8. Given we always align on a
"natural" boundary, I believe the bit_offset will always be 32 bits.
A comment here might help clarify why we do this as well in case folks
don't see the change description.
> + uaddr &= ~(sizeof(unsigned long) - 1);
Shouldn't this uaddr change be done before calling pin_user_pages_remote()
to ensure things cannot go bad? (I don't think they can, but it looks a
little odd).
Thanks,
-Beau
> + }
> ptr = kaddr + (uaddr & ~PAGE_MASK);
>
> /* Update bit atomically, user tracers must be atomic as well */
> if (enabler->event && enabler->event->status)
> - set_bit(ENABLE_BIT(enabler), ptr);
> + set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
> else
> - clear_bit(ENABLE_BIT(enabler), ptr);
> + clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
>
> kunmap_local(kaddr);
> unpin_user_pages_dirty_lock(, 1, true);
> --
> 2.40.1
[PATCH 1/2 v3] eventfs: Remove eventfs_file and just use eventfs_inode
From: "Steven Rostedt (Google)"
Instead of having a descriptor for every file represented in the eventfs
directory, only have the directory itself represented. Change the API to
send in a list of entries that represent all the files in the directory
(but not other directories). The entry list contains a name and a callback
function that will be used to create the files when they are accessed.
struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
*parent,
const struct eventfs_entry
*entries,
int size, void *data);
is used for the top level eventfs directory, and returns an eventfs_inode
that will be used by:
struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
*parent,
const struct eventfs_entry *entries,
int size, void *data);
where both of the above take an array of struct eventfs_entry entries for
every file that is in the directory.
The entries are defined by:
typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
const struct file_operations **fops);
struct eventfs_entry {
const char *name;
eventfs_callbackcallback;
};
Where the name is the name of the file and the callback gets called when
the file is being created. The callback passes in the name (in case the
same callback is used for multiple files), a pointer to the mode, data and
fops. The data will be pointing to the data that was passed in
eventfs_create_dir() or eventfs_create_events_dir() but may be overridden
to point to something else, as it will be used to point to the
inode->i_private that is created. The information passed back from the
callback is used to create the dentry/inode.
If the callback fills the data and the file should be created, it must
return a positive number. On zero or negative, the file is ignored.
This logic may also be used as a prototype to convert entire pseudo file
systems into just-in-time allocation.
The "show_events_dentry" file has been updated to show the directories,
and any files they have.
With just the eventfs_file allocations:
Before after deltas for meminfo (in kB):
MemFree: -14360
MemAvailable:-14260
Buffers: 40
Cached: 24
Active: 44
Inactive:48
Inactive(anon): 28
Active(file):44
Inactive(file): 20
Dirty: -4
AnonPages: 28
Mapped: 4
KReclaimable:132
Slab:1604
SReclaimable:132
SUnreclaim: 1472
Committed_AS:12
Before after deltas for slabinfo:
: [ * = ]
ext4_inode_cache 27 [* 1184 = 31968 ]
extent_status102 [* 40 = 4080 ]
tracefs_inode_cache 144 [* 656 = 94464 ]
buffer_head 39 [* 104 = 4056 ]
shmem_inode_cache49 [* 800 = 39200 ]
filp -53 [* 256 = -13568 ]
dentry 251 [* 192 = 48192 ]
lsm_file_cache 277 [* 32 = 8864 ]
vm_area_struct -14 [* 184 = -2576 ]
trace_event_file 1748[* 88 = 153824 ]
kmalloc-1k 35 [* 1024 = 35840 ]
kmalloc-256 49 [* 256 = 12544 ]
kmalloc-192 -28 [* 192 = -5376 ]
kmalloc-128 -30 [* 128 = -3840 ]
kmalloc-96 10581 [* 96 = 1015776 ]
kmalloc-64 3056[* 64 = 195584 ]
kmalloc-32 1291[* 32 = 41312 ]
kmalloc-16 2310[* 16 = 36960 ]
kmalloc-89216[*8 = 73728 ]
Free memory dropped by 14,360 kB
Available memory dropped by 14,260 kB
Total slab additions in size: 1,771,032 bytes
With this change:
Before after deltas for meminfo (in kB):
MemFree: -12084
MemAvailable:-11976
Buffers: 32
Cached: 32
Active: 72
Inactive:168
Inactive(anon): 176
Active(file):72
Inactive(file): -8
Dirty: 24
AnonPages: 196
Mapped: 8
KReclaimable:148
Slab:836
SReclaimable:148
SUnreclaim: 688
Committed_AS:324
Before after deltas for slabinfo:
: [ * = ]
tracefs_inode_cache 144 [* 656 = 94464 ]
shmem_inode_cache-23 [* 800 = -18400 ]
filp -92 [* 256 = -23552 ]
dentry 179 [* 192 = 34368 ]
lsm_file_cache -3
[PATCH 0/2 v3] tracing: Remove eventfs_files by use of callbacks
This is a bit of a redesign of the way the eventfs is created. It no longer creates a descriptor representing every file but instead just the directories. These descriptors get an array of entries that represent the files within it (but not for sub directories). Each entry has a name and a callback, where the name is the name of the file (used for lookup) and a callback that is used to create the dentry and inode for the file. This saves more memory, this approach may be possible to create a dynamic way of doing this for other pseudo file systems. The second patch fixes the kprobe selftests yet again, and the function that it uses to attach to was renamed once again. Changes since v2: https://lore.kernel.org/all/20230913025855.615399...@goodmis.org/ - Removed the show_eventfs_dentries patch and queued that for mainline. - Rebased on top of the queued mainline tree, as the show_eventfs_dentries patch was modified. - Updated the numbers in the change log to reflect the latest code. (Things actually got better!) Changes since v1: https://lore.kernel.org/all/20230801001349.520930...@goodmis.org/ - Rebased on mainline and some minor clean ups. - Fixed kprobe selftest Steven Rostedt (Google) (2): eventfs: Remove eventfs_file and just use eventfs_inode tracing/selftests: Update kprobe args char/string to match new functions fs/tracefs/event_inode.c | 763 +++-- fs/tracefs/event_show.c| 51 +- fs/tracefs/inode.c | 2 +- fs/tracefs/internal.h | 54 +- include/linux/trace_events.h | 2 +- include/linux/tracefs.h| 29 +- kernel/trace/trace.c | 7 +- kernel/trace/trace.h | 4 +- kernel/trace/trace_events.c| 315 ++--- .../ftrace/test.d/kprobe/kprobe_args_char.tc | 4 +- .../ftrace/test.d/kprobe/kprobe_args_string.tc | 4 +- 11 files changed, 691 insertions(+), 544 deletions(-)
[PATCH 2/2 v3] tracing/selftests: Update kprobe args char/string to match new functions
From: "Steven Rostedt (Google)" The function that the kprobe_args_char and kprobes_arg_string attaches to for its test has changed its name once again. Now we need to check for eventfs_create_dir(), and if it exists, use that, otherwise check for eventfs_add_dir() and if that exists use that, otherwise use the original tracefs_create_dir()! Acked-by: Masami Hiramatsu (Google) Signed-off-by: Steven Rostedt (Google) --- .../selftests/ftrace/test.d/kprobe/kprobe_args_char.tc| 4 +++- .../selftests/ftrace/test.d/kprobe/kprobe_args_string.tc | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_char.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_char.tc index ff7499eb98d6..c639c6c8ca03 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_char.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_char.tc @@ -34,7 +34,9 @@ mips*) esac : "Test get argument (1)" -if grep -q eventfs_add_dir available_filter_functions; then +if grep -q eventfs_create_dir available_filter_functions; then + DIR_NAME="eventfs_create_dir" +elif grep -q eventfs_add_dir available_filter_functions; then DIR_NAME="eventfs_add_dir" else DIR_NAME="tracefs_create_dir" diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_string.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_string.tc index a202b2ea4baf..a5ab4d5c74ac 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_string.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_string.tc @@ -37,7 +37,9 @@ loongarch*) esac : "Test get argument (1)" -if grep -q eventfs_add_dir available_filter_functions; then +if grep -q eventfs_create_dir available_filter_functions; then + DIR_NAME="eventfs_create_dir" +elif grep -q eventfs_add_dir available_filter_functions; then DIR_NAME="eventfs_add_dir" else DIR_NAME="tracefs_create_dir" -- 2.40.1
[PATCH] ring_buffer: Use try_cmpxchg instead of cmpxchg in rb_insert_pages
Use try_cmpxchg instead of cmpxchg (*ptr, old, new) == old in
rb_insert_pages. x86 CMPXCHG instruction returns success in ZF flag,
so this change saves a compare after cmpxchg (and related move
instruction in front of cmpxchg).
No functional change intended.
Cc: Steven Rostedt
Cc: Masami Hiramatsu
Signed-off-by: Uros Bizjak
---
kernel/trace/ring_buffer.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index a1651edc48d5..3e2a6478425c 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2048,7 +2048,7 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer)
retries = 10;
success = false;
while (retries--) {
- struct list_head *head_page, *prev_page, *r;
+ struct list_head *head_page, *prev_page;
struct list_head *last_page, *first_page;
struct list_head *head_page_with_bit;
struct buffer_page *hpage = rb_set_head_page(cpu_buffer);
@@ -2067,9 +2067,9 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer)
last_page->next = head_page_with_bit;
first_page->prev = prev_page;
- r = cmpxchg(_page->next, head_page_with_bit, first_page);
-
- if (r == head_page_with_bit) {
+ /* caution: head_page_with_bit gets updated on cmpxchg failure
*/
+ if (try_cmpxchg(_page->next,
+ _page_with_bit, first_page)) {
/*
* yay, we replaced the page pointer to our new list,
* now, we just have to update to head page's prev
--
2.41.0
Re: [PATCH v4 03/18] x86/sgx: Add sgx_epc_lru_lists to encapsulate LRU lists
On 9/14/23 03:31, Huang, Kai wrote: >> Signed-off-by: Haitao Huang >> Cc: Sean Christopherson > You don't need 'Cc:' Sean if the patch has Sean's SoB. It is a SoB for Sean's @intel address and cc's his @google address. It is fine.
Re: (subset) [PATCH 00/11] Initial support for the Fairphone 5 smartphone
On Wed, 30 Aug 2023 11:58:25 +0200, Luca Weiss wrote: > Add support to boot up mainline kernel on the QCM6490-based Fairphone 5 > smartphone. > > These patches only cover a part of the functionality brought up on > mainline so far, with the rest needing larger dts and driver changes or > depend on patches that are not yet merged. I will work on sending those > once these base patches here have settled. > > [...] Applied, thanks! [07/11] dt-bindings: arm: qcom,ids: Add SoC ID for QCM6490 commit: ccfb4d8b606302d857a03ea29039e21029311335 [08/11] soc: qcom: socinfo: Add SoC ID for QCM6490 commit: 59872d59d164ec67f295d6f96fe818b92973ee40 Best regards, -- Bjorn Andersson
Re: (subset) [PATCH v3 0/7] MSM8976 PLL,RPMPD and DTS changes
On Sat, 12 Aug 2023 13:24:43 +0200, Adam Skladowski wrote: > This patch series fixes introduce support for msm8976 pll, > also brings some adjustments and fixes domains setup and few dts nitpicks. > > Changes since v1 > > 1. Fixed few styling issues > 2. Changed compatibles for plls > 3. Added fixes: tag to first patch > > [...] Applied, thanks! [2/7] clk: qcom: clk-hfpll: Configure l_val in init when required commit: 500a4609eef46d49a260173b66cabb20bd5159ad [3/7] clk: qcom: hfpll: Allow matching pdata commit: 34e000c0963e55f24be2254fa645f8dd8257a9e0 [4/7] dt-bindings: clock: qcom,hfpll: Document MSM8976 compatibles commit: de37ca2dc98607e74522d8f243aa7feac74577c5 [5/7] clk: qcom: hfpll: Add MSM8976 PLL data commit: 1fa2d1a887c763246662a88e203d69b36052770c Best regards, -- Bjorn Andersson
Re: [PATCH v3] libnvdimm/of_pmem: Use devm_kstrdup instead of kstrdup and check its return value
On 9/14/23 00:03, Chen Ni wrote:
> Use devm_kstrdup() instead of kstrdup() and check its return value to
> avoid memory leak.
>
> Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus
> provider")
> Signed-off-by: Chen Ni
Reviewed-by: Dave Jiang
One unrelated comment below.
> ---
> Changelog:
>
> v2 -> v3:
>
> 1. Use devm_kstrdup() instead of kstrdup()
>
> v1 -> v2:
>
> 1. Add a fixes tag.
> 2. Update commit message.
> ---
> drivers/nvdimm/of_pmem.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index 1b9f5b8a6167..5765674b36f2 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -30,7 +30,13 @@ static int of_pmem_region_probe(struct platform_device
> *pdev)
> if (!priv)
> return -ENOMEM;
>
> - priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> + priv->bus_desc.provider_name = devm_kstrdup(>dev, pdev->name,
> + GFP_KERNEL);
> + if (!priv->bus_desc.provider_name) {
> + kfree(priv);
I wonder if priv should be allocated with devm_kzalloc() instead to reduce the
resource management burden.
> + return -ENOMEM;
> + }
> +
> priv->bus_desc.module = THIS_MODULE;
> priv->bus_desc.of_node = np;
>
Re: [PATCH v6 1/4] dt-bindings: remoteproc: k3-m4f: Add K3 AM64x SoCs
On Wed, Sep 13, 2023 at 6:17 AM Hari Nagalla wrote: > > K3 AM64x SoC has a Cortex M4F subsystem in the MCU voltage domain. > The remote processor's life cycle management and IPC mechanisms are > similar across the R5F and M4F cores from remote processor driver > point of view. However, there are subtle differences in image loading > and starting the M4F subsystems. > > The YAML binding document provides the various node properties to be > configured by the consumers of the M4F subsystem. > > Signed-off-by: Martyn Welch > Signed-off-by: Hari Nagalla > --- > Changes since v1: > - Spelling corrections > - Corrected to pass DT checks > > Changes since v2: > - Missed spelling correction to commit message > > Changes since v3: > - Removed unnecessary descriptions and used generic memory region names > - Made mboxes and memory-region optional > - Removed unrelated items from examples > > Changes since v4: > - Rebased to the latest kernel-next tree > - Added optional sram memory region for m4f device node > > Changes since v5: > - None > > .../bindings/remoteproc/ti,k3-m4f-rproc.yaml | 136 ++ > 1 file changed, 136 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml > > diff --git > a/Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml > b/Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml > new file mode 100644 > index ..21b7f14d9dc4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-m4f-rproc.yaml > @@ -0,0 +1,136 @@ > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/remoteproc/ti,k3-m4f-rproc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: TI K3 M4F processor subsystems > + > +maintainers: > + - Hari Nagalla > + - Mathieu Poirier > + > +description: | > + Some K3 family SoCs have Arm Cortex M4F cores. AM64x is a SoC in K3 > + family with a M4F core. Typically safety oriented applications may use > + the M4F core in isolation without an IPC. Where as some industrial and > + home automation applications, may use the M4F core as a remote processor > + with IPC communications. > + > +$ref: /schemas/arm/keystone/ti,k3-sci-common.yaml# > + > +properties: > + > + compatible: > +enum: > + - ti,am64-m4fss > + > + power-domains: > +maxItems: 1 > + > + "#address-cells": > +const: 2 > + > + "#size-cells": > +const: 2 > + > + reg: > +items: > + - description: IRAM internal memory region > + - description: DRAM internal memory region > + > + reg-names: > +items: > + - const: iram > + - const: dram > + > + resets: > +maxItems: 1 > + > + firmware-name: > +$ref: /schemas/types.yaml#/definitions/string > +description: Name of firmware to load for the M4F core > + > + mboxes: > +description: | > + Mailbox specifier denoting the sub-mailbox, to be used for > communication > + with the remote processor. This property should match with the > + sub-mailbox node used in the firmware image. > +maxItems: 2 > + > + memory-region: > +description: | > + phandle to the reserved memory nodes to be associated with the > + remoteproc device. The reserved memory nodes should be carveout nodes, > + and should be defined with a "no-map" property as per the bindings in > + Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml > + Optional memory regions available for firmware specific purposes. > +maxItems: 8 > +items: > + - description: regions used for DMA allocations like vrings, vring > buffers > + and memory dedicated to firmware's specific purposes. > +additionalItems: true > + > + sram: > +$ref: /schemas/types.yaml#/definitions/phandle-array > +minItems: 1 > +maxItems: 4 > +items: > + maxItems: 4 You are saying there are 1-4 entries and each entry is 4 cells. What's in the 4 cells? >From the description and example, looks like you only have 1 cell (a phandle) so maxItems should be 1. Your example should fail, but I'm not sure why it doesn't. Rob
Re: [PATCH] x86/hyperv: Restrict get_vtl to only VTL platforms
Saurabh Sengar writes:
> For non VTL platforms vtl is always 0, and there is no need of
> get_vtl function. For VTL platforms get_vtl should always succeed
> and should return the correct VTL.
>
> Signed-off-by: Saurabh Sengar
> ---
> arch/x86/hyperv/hv_init.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 783ed339f341..e589c240565a 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -416,8 +416,8 @@ static u8 __init get_vtl(void)
> if (hv_result_success(ret)) {
> ret = output->as64.low & HV_X64_VTL_MASK;
> } else {
> - pr_err("Failed to get VTL(%lld) and set VTL to zero by
> default.\n", ret);
> - ret = 0;
> + pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
Nitpick: arch/x86/hyperv/hv_init.c lacks pr_fmt so the message won't get
prefixed with "Hyper-V". I'm not sure 'VTL' abbreviation has the only,
Hyper-V specific meaning. I'd suggest we add
#define pr_fmt(fmt) "Hyper-V: " fmt
to the beginning of the file.
> + BUG();
> }
>
> local_irq_restore(flags);
> @@ -604,8 +604,10 @@ void __init hyperv_init(void)
> hv_query_ext_cap(0);
>
> /* Find the VTL */
> - if (!ms_hyperv.paravisor_present && hv_isolation_type_snp())
> + if (IS_ENABLED(CONFIG_HYPERV_VTL_MODE))
> ms_hyperv.vtl = get_vtl();
> + else
> + ms_hyperv.vtl = 0;
Is 'else' branch really needed? 'ms_hyperv' seems to be a statically
allocated global. But instead of doing this, what about putting the
whole get_vtl() funtion under '#if (IS_ENABLED(CONFIG_HYPERV_VTL_MODE))', i.e.:
#if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
static u8 __init get_vtl(void)
{
u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
...
}
#else
static inline get_vtl(void) { return 0; }
#endif
and then we can always do
ms_hyperv.vtl = get_vtl();
unconditionally?
>
> return;
--
Vitaly
Re: [PATCH 2/8] usb: gadget: add anonymous definition in some struct for trace purpose
You didn't include the version number in the Subject: line. Undoubtedly Greg's automatic error checker will warn you about this. Unless the version number is clearly marked for each patch, it's difficult for his programs to tell which email message contains the most recent version. On Thu, Sep 14, 2023 at 06:02:56PM +0800, Linyu Yuan wrote: > Some UDC trace event will save usb udc information, but it use one int > size buffer to save one bit information of usb udc, it is wast trace > buffer. > > Add anonymous union which have one u32 member can be used by trace event > during fast assign stage to save more entries with same trace ring buffer > size. > > Signed-off-by: Linyu Yuan > --- And you didn't include the version change information here, below the "---" line. Apart from that, this is a _lot_ better than before! I don't know if Greg will think this change is worth merging, but at least now it's possible to read the code and understand what's going on. Alan Stern
Re: [PATCH 1/8] trace: add new DECLARE_EVENT_CLASS_PRINT_INIT class type
Hi Linyu, kernel test robot noticed the following build warnings: [auto build test WARNING on usb/usb-testing] [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.6-rc1 next-20230914] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Linyu-Yuan/trace-add-new-DECLARE_EVENT_CLASS_PRINT_INIT-class-type/20230914-180924 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/20230914100302.30274-2-quic_linyyuan%40quicinc.com patch subject: [PATCH 1/8] trace: add new DECLARE_EVENT_CLASS_PRINT_INIT class type config: arm-defconfig (https://download.01.org/0day-ci/archive/20230914/202309142216.gwm6q6l0-...@intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/202309142216.gwm6q6l0-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202309142216.gwm6q6l0-...@intel.com/ All warnings (new ones prefixed by >>): In file included from include/trace/events/migrate.h:8, from mm/rmap.c:83: >> include/linux/tracepoint.h:554: warning: "DECLARE_EVENT_CLASS_PRINT_INIT" >> redefined 554 | #define DECLARE_EVENT_CLASS_PRINT_INIT(name, proto, args, tstruct, assign, print, init) | In file included from include/trace/define_trace.h:103, from include/trace/events/tlb.h:62, from mm/rmap.c:82: include/trace/perf.h:59: note: this is the location of the previous definition 59 | #define DECLARE_EVENT_CLASS_PRINT_INIT(call, proto, args, tstruct, assign, print, init) \ | >> include/linux/tracepoint.h:580: warning: "TRACE_EVENT_PRINT_INIT" redefined 580 | #define TRACE_EVENT_PRINT_INIT(name, proto, args, struct, assign, print, init) \ | In file included from include/trace/define_trace.h:102: include/trace/trace_events.h:49: note: this is the location of the previous definition 49 | #define TRACE_EVENT_PRINT_INIT(name, proto, args, tstruct, assign, print, init) \ | -- In file included from include/trace/events/net.h:12, from net/core/net-traces.c:31: >> include/linux/tracepoint.h:554: warning: "DECLARE_EVENT_CLASS_PRINT_INIT" >> redefined 554 | #define DECLARE_EVENT_CLASS_PRINT_INIT(name, proto, args, tstruct, assign, print, init) | In file included from include/trace/define_trace.h:103, from include/trace/events/skb.h:95, from net/core/net-traces.c:30: include/trace/perf.h:59: note: this is the location of the previous definition 59 | #define DECLARE_EVENT_CLASS_PRINT_INIT(call, proto, args, tstruct, assign, print, init) \ | >> include/linux/tracepoint.h:580: warning: "TRACE_EVENT_PRINT_INIT" redefined 580 | #define TRACE_EVENT_PRINT_INIT(name, proto, args, struct, assign, print, init) \ | In file included from include/trace/define_trace.h:102: include/trace/trace_events.h:49: note: this is the location of the previous definition 49 | #define TRACE_EVENT_PRINT_INIT(name, proto, args, tstruct, assign, print, init) \ | In file included from include/trace/events/napi.h:9, from net/core/net-traces.c:32: >> include/linux/tracepoint.h:554: warning: "DECLARE_EVENT_CLASS_PRINT_INIT" >> redefined 554 | #define DECLARE_EVENT_CLASS_PRINT_INIT(name, proto, args, tstruct, assign, print, init) | In file included from include/trace/define_trace.h:103, from include/trace/events/net.h:319: include/trace/perf.h:59: note: this is the location of the previous definition 59 | #define DECLARE_EVENT_CLASS_PRINT_INIT(call, proto, args, tstruct, assign, print, init) \ | >> include/linux/tracepoint.h:580: warning: "TRACE_EVENT_PRINT_INIT" redefined 580 | #define TRACE_EVENT_PRINT_INIT(name, proto, args, struct, assign, print, init) \ | In file included from include/trace/define_trace.h:102: include/trace/trace_events.h:49: note: this is the location of the previous definition 49 | #define TRACE_EVENT_PRINT_INIT(name, proto, args, tstruct, assign, print, init) \ | In file included from include/trace/events/sock.h:10, from net/core/net-traces.c:33: >> include/linux/tracepoint.h:554: warning: "DECLARE_EVENT_CLASS_PRINT_INIT"
Re: [PATCH 03/19] fs: release anon dev_t in deactivate_locked_super
> Christoph, could you explain what the hell do we need that for? It does > create the race in question and AFAICS 2c18a63b760a (and followups trying > to plug holes in it) had been nothing but headache. > > Old logics: if mount attempt with a different fs type happens, -EBUSY > is precisely corrent - we would've gotten just that if mount() came > before umount(). If the type matches, we might > 1) come before deactivate_locked_super() by umount(2). > No problem, we succeed. > 2) come after the beginning of shutdown, but before the > removal from the list; fine, we'll wait for the sucker to be > unlocked (which happens in the end of generic_shutdown_super()), > notice it's dead and create a new superblock. Since the only > part left on the umount side is closing the device, we are > just fine. > 3) come after the removal from the list. So we won't > wait for the old superblock to be unlocked, other than that > it's exactly the same as (2). It doesn't matter whether we > open the device before or after close by umount - same owner > anyway, no -EBUSY. > > Your "owner shall be the superblock" breaks that... > > If you want to mess with _three_-way split of ->kill_sb(), > please start with writing down the rules re what should > go into each of those parts; such writeup should go into > Documentation/filesystems/porting anyway, even if the > split is a two-way one, BTW. Hm, I think that characterization of Christoph's changes is a bit harsh. Yes, you're right that making the superblock and not the filesytem type the bd_holder changes the logic and we are aware of that of course. And it requires changes such as moving additional block device closing from where some callers currently do it. But the filesytem type is not a very useful holder itself and has other drawbacks. The obvious one being that it requires us to wade through all superblocks on the system trying to find the superblock associated with a given block device continously grabbing and dropping sb_lock and s_umount. None of that is very pleasant nor elegant and it is for sure not very easy to understand (Plus, it's broken for btrfs freezing and syncing via block level ioctls.). Using the superblock as holder makes this go away and is overall a lot more useful and intuitive and can be extended to filesystems with multiple devices (Of which we apparently are bound to get more.). So I think this change is worth the pain. It's a fair point that these lifetime rules should be documented in Documentation/filesystems/. The old lifetime documentation is too sparse to be useful though.
[PATCH 2/2] mm, vmscan: remove ISOLATE_UNMAPPED
This isolate_mode_t flag is effectively unused since 89f6c88a6ab4 ("mm:
__isolate_lru_page_prepare() in isolate_migratepages_block()") as
sc->may_unmap is now checked directly (and only node_reclaim has a mode
that sets it to 0). The last remaining place is mm_vmscan_lru_isolate
tracepoint for the isolate_mode parameter. That one was mainly used to
indicate the active/inactive mode, which the trace-vmscan-postprocess.pl
script consumed, but that got silently broken. After fixing the script
by the previous patch, it does not need the isolate_mode anymore. So
just remove the parameter and with that the whole ISOLATE_UNMAPPED flag.
Signed-off-by: Vlastimil Babka
---
.../trace/postprocess/trace-vmscan-postprocess.pl | 8
include/linux/mmzone.h| 2 --
include/trace/events/vmscan.h | 8 ++--
mm/vmscan.c | 3 +--
4 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
index 725d41a8d4ef..048dc0dbce64 100644
--- a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
+++ b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
@@ -112,7 +112,7 @@ my $regex_direct_end_default = 'nr_reclaimed=([0-9]*)';
my $regex_kswapd_wake_default = 'nid=([0-9]*) order=([0-9]*)';
my $regex_kswapd_sleep_default = 'nid=([0-9]*)';
my $regex_wakeup_kswapd_default = 'nid=([0-9]*) order=([0-9]*)
gfp_flags=([A-Z_|]*)';
-my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) classzone=([0-9]*)
order=([0-9]*) nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*)
nr_taken=([0-9]*) lru=([a-z_]*)';
+my $regex_lru_isolate_default = 'classzone=([0-9]*) order=([0-9]*)
nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*) nr_taken=([0-9]*)
lru=([a-z_]*)';
my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) nr_scanned=([0-9]*)
nr_reclaimed=([0-9]*) nr_dirty=([0-9]*) nr_writeback=([0-9]*)
nr_congested=([0-9]*) nr_immediate=([0-9]*) nr_activate_anon=([0-9]*)
nr_activate_file=([0-9]*) nr_ref_keep=([0-9]*) nr_unmap_fail=([0-9]*)
priority=([0-9]*) flags=([A-Z_|]*)';
my $regex_lru_shrink_active_default = 'lru=([A-Z_]*) nr_taken=([0-9]*)
nr_active=([0-9]*) nr_deactivated=([0-9]*) nr_referenced=([0-9]*)
priority=([0-9]*) flags=([A-Z_|]*)' ;
my $regex_writepage_default = 'page=([0-9a-f]*) pfn=([0-9]*) flags=([A-Z_|]*)';
@@ -204,7 +204,7 @@ $regex_wakeup_kswapd = generate_traceevent_regex(
$regex_lru_isolate = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_isolate",
$regex_lru_isolate_default,
- "isolate_mode", classzone", "order",
+ "classzone", "order",
"nr_requested", "nr_scanned", "nr_skipped", "nr_taken",
"lru");
$regex_lru_shrink_inactive = generate_traceevent_regex(
@@ -379,8 +379,8 @@ sub process_events {
print " $regex_lru_isolate/o\n";
next;
}
- my $nr_scanned = $5;
- my $lru = $8;
+ my $nr_scanned = $4;
+ my $lru = $7;
# To closer match vmstat scanning statistics, only count
# inactive lru as scanning
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 4106fbc5b4b3..486587fcd27f 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -639,8 +639,6 @@ struct lruvec {
#endif
};
-/* Isolate unmapped pages */
-#define ISOLATE_UNMAPPED ((__force isolate_mode_t)0x2)
/* Isolate for asynchronous migration */
#define ISOLATE_ASYNC_MIGRATE ((__force isolate_mode_t)0x4)
/* Isolate unevictable pages */
diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
index d2123dd960d5..1a488c30afa5 100644
--- a/include/trace/events/vmscan.h
+++ b/include/trace/events/vmscan.h
@@ -285,10 +285,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
unsigned long nr_scanned,
unsigned long nr_skipped,
unsigned long nr_taken,
- isolate_mode_t isolate_mode,
int lru),
- TP_ARGS(highest_zoneidx, order, nr_requested, nr_scanned, nr_skipped,
nr_taken, isolate_mode, lru),
+ TP_ARGS(highest_zoneidx, order, nr_requested, nr_scanned, nr_skipped,
nr_taken, lru),
TP_STRUCT__entry(
__field(int, highest_zoneidx)
@@ -297,7 +296,6 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
__field(unsigned long, nr_scanned)
__field(unsigned long, nr_skipped)
__field(unsigned long, nr_taken)
- __field(unsigned int, isolate_mode)
__field(int, lru)
),
@@ -308,7 +306,6 @@
[PATCH 1/2] trace-vmscan-postprocess: sync with tracepoints updates
The script has fallen behind tracepoint changes for a while, fix it up.
Most changes are mechanical (renames, removal of tracepoint parameters
that are not used by the script). More notable change involves
mm_vmscan_lru_isolate which is relying on the isolate_mode to determine
if the inactive list is being scanned. However the parameter currently
only indicates ISOLATE_UNMAPPED. We can use the lru parameter instead to
determine which list is scanned, and stop checking isolate_mode.
Signed-off-by: Vlastimil Babka
---
.../postprocess/trace-vmscan-postprocess.pl | 40 ---
1 file changed, 17 insertions(+), 23 deletions(-)
diff --git a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
index e24c009789a0..725d41a8d4ef 100644
--- a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
+++ b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
@@ -107,14 +107,14 @@ GetOptions(
);
# Defaults for dynamically discovered regex's
-my $regex_direct_begin_default = 'order=([0-9]*) may_writepage=([0-9]*)
gfp_flags=([A-Z_|]*)';
+my $regex_direct_begin_default = 'order=([0-9]*) gfp_flags=([A-Z_|]*)';
my $regex_direct_end_default = 'nr_reclaimed=([0-9]*)';
my $regex_kswapd_wake_default = 'nid=([0-9]*) order=([0-9]*)';
my $regex_kswapd_sleep_default = 'nid=([0-9]*)';
-my $regex_wakeup_kswapd_default = 'nid=([0-9]*) zid=([0-9]*) order=([0-9]*)
gfp_flags=([A-Z_|]*)';
-my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) classzone_idx=([0-9]*)
order=([0-9]*) nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*)
nr_taken=([0-9]*) lru=([a-z_]*)';
+my $regex_wakeup_kswapd_default = 'nid=([0-9]*) order=([0-9]*)
gfp_flags=([A-Z_|]*)';
+my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) classzone=([0-9]*)
order=([0-9]*) nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*)
nr_taken=([0-9]*) lru=([a-z_]*)';
my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) nr_scanned=([0-9]*)
nr_reclaimed=([0-9]*) nr_dirty=([0-9]*) nr_writeback=([0-9]*)
nr_congested=([0-9]*) nr_immediate=([0-9]*) nr_activate_anon=([0-9]*)
nr_activate_file=([0-9]*) nr_ref_keep=([0-9]*) nr_unmap_fail=([0-9]*)
priority=([0-9]*) flags=([A-Z_|]*)';
-my $regex_lru_shrink_active_default = 'lru=([A-Z_]*) nr_scanned=([0-9]*)
nr_rotated=([0-9]*) priority=([0-9]*)';
+my $regex_lru_shrink_active_default = 'lru=([A-Z_]*) nr_taken=([0-9]*)
nr_active=([0-9]*) nr_deactivated=([0-9]*) nr_referenced=([0-9]*)
priority=([0-9]*) flags=([A-Z_|]*)' ;
my $regex_writepage_default = 'page=([0-9a-f]*) pfn=([0-9]*) flags=([A-Z_|]*)';
# Dyanically discovered regex
@@ -184,8 +184,7 @@ sub generate_traceevent_regex {
$regex_direct_begin = generate_traceevent_regex(
"vmscan/mm_vmscan_direct_reclaim_begin",
$regex_direct_begin_default,
- "order", "may_writepage",
- "gfp_flags");
+ "order", "gfp_flags");
$regex_direct_end = generate_traceevent_regex(
"vmscan/mm_vmscan_direct_reclaim_end",
$regex_direct_end_default,
@@ -201,11 +200,11 @@ $regex_kswapd_sleep = generate_traceevent_regex(
$regex_wakeup_kswapd = generate_traceevent_regex(
"vmscan/mm_vmscan_wakeup_kswapd",
$regex_wakeup_kswapd_default,
- "nid", "zid", "order", "gfp_flags");
+ "nid", "order", "gfp_flags");
$regex_lru_isolate = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_isolate",
$regex_lru_isolate_default,
- "isolate_mode", "classzone_idx", "order",
+ "isolate_mode", classzone", "order",
"nr_requested", "nr_scanned", "nr_skipped", "nr_taken",
"lru");
$regex_lru_shrink_inactive = generate_traceevent_regex(
@@ -218,11 +217,10 @@ $regex_lru_shrink_inactive = generate_traceevent_regex(
$regex_lru_shrink_active = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_shrink_active",
$regex_lru_shrink_active_default,
- "nid", "zid",
- "lru",
- "nr_scanned", "nr_rotated", "priority");
+ "nid", "nr_taken", "nr_active", "nr_deactivated",
"nr_referenced",
+ "priority", "flags");
$regex_writepage = generate_traceevent_regex(
- "vmscan/mm_vmscan_writepage",
+ "vmscan/mm_vmscan_write_folio",
$regex_writepage_default,
"page", "pfn", "flags");
@@ -371,7 +369,7 @@ sub process_events {
print " $regex_wakeup_kswapd\n";
next;
}
- my $order = $3;
[PATCH] tracing/user_events: align uaddr on unsigned long alignment
enabler->uaddr can be aligned on 32 or 64 bits. If aligned on 32 bits,
this will result in a misaligned access on 64 bits architectures since
set_bit()/clear_bit() are expecting an unsigned long (aligned) pointer.
On architecture that do not support misaligned access, this will crash
the kernel. Align uaddr on unsigned long size to avoid such behavior.
This bug was found while running kselftests on RISC-V.
Fixes: 7235759084a4 ("tracing/user_events: Use remote writes for event
enablement")
Signed-off-by: Clément Léger
---
kernel/trace/trace_events_user.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index 6f046650e527..580c0fe4b23e 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -479,7 +479,7 @@ static int user_event_enabler_write(struct user_event_mm
*mm,
bool fixup_fault, int *attempt)
{
unsigned long uaddr = enabler->addr;
- unsigned long *ptr;
+ unsigned long *ptr, bit_offset;
struct page *page;
void *kaddr;
int ret;
@@ -511,13 +511,19 @@ static int user_event_enabler_write(struct user_event_mm
*mm,
}
kaddr = kmap_local_page(page);
+
+ bit_offset = uaddr & (sizeof(unsigned long) - 1);
+ if (bit_offset) {
+ bit_offset *= 8;
+ uaddr &= ~(sizeof(unsigned long) - 1);
+ }
ptr = kaddr + (uaddr & ~PAGE_MASK);
/* Update bit atomically, user tracers must be atomic as well */
if (enabler->event && enabler->event->status)
- set_bit(ENABLE_BIT(enabler), ptr);
+ set_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
else
- clear_bit(ENABLE_BIT(enabler), ptr);
+ clear_bit(ENABLE_BIT(enabler) + bit_offset, ptr);
kunmap_local(kaddr);
unpin_user_pages_dirty_lock(, 1, true);
--
2.40.1
Re: [PATCH v2] verification/dot2k: Delete duplicate imports
On 9/6/23 17:57, Alessandro Carminati (Red Hat) wrote:
> The presence of duplicate import lines appears to be a typo.
> Removing them.
>
> Fixes: 24bce201d798 ("tools/rv: Add dot2k")
> Signed-off-by: Alessandro Carminati (Red Hat)
Queued, thanks!
https://git.kernel.org/pub/scm/linux/kernel/git/bristot/linux.git/commit/?h=tools/verification=5a9587fea055163026b6d22d593fc64ed04de3a6
-- Daniel
Re: [PATCH v4 03/18] x86/sgx: Add sgx_epc_lru_lists to encapsulate LRU lists
Some non-technical staff: On Tue, 2023-09-12 at 21:06 -0700, Haitao Huang wrote: > From: Kristen Carlson Accardi The patch was from Kristen, but ... > > Introduce a data structure to wrap the existing reclaimable list and its > spinlock. Each cgroup later will have one instance of this structure to > track EPC pages allocated for processes associated with the same cgroup. > Just like the global SGX reclaimer (ksgxd), an EPC cgroup reclaims pages > from the reclaimable list in this structure when its usage reaches near > its limit. > > Currently, ksgxd does not track the VA, SECS pages. They are considered > as 'unreclaimable' pages that are only deallocated when their respective > owning enclaves are destroyed and all associated resources released. > > When an EPC cgroup can not reclaim any more reclaimable EPC pages to > reduce its usage below its limit, the cgroup must also reclaim those > unreclaimables by killing their owning enclaves. The VA and SECS pages > later are also tracked in an 'unreclaimable' list added to this structure > to support this OOM killing of enclaves. > > Signed-off-by: Sean Christopherson > Signed-off-by: Kristen Carlson Accardi ... it was firstly signed by Sean and then Kristen, which doesn't sound right. If the patch was from Kristen, then either Sean's SoB should come after Kristen's (which means Sean took Kristen's patch and signed it), or you need to have a Co-developed-by tag for Sean right before his SoB (which indicates Sean participated in the development of the patch but likely he wasn't the main developer). But I _guess_ the patch was just from Sean. > Signed-off-by: Haitao Huang > Cc: Sean Christopherson You don't need 'Cc:' Sean if the patch has Sean's SoB. More information please refer to "When to use Acked-by:, Cc:, and Co-developed- by" section here: https://docs.kernel.org/process/submitting-patches.html Also an explanation of when to use 'Cc:' from Sean (ignore technical staff): https://lore.kernel.org/lkml/zozteoxjvq9v6...@google.com/ (And please check other patches too.)
Re: [PATCH 10/19] USB: gadget/legacy: remove sb_mutex
On 9/13/23 2:10 PM, Christoph Hellwig wrote: > Creating new a new super_block vs freeing the old one for single instance ^ I can't parse that. :-) > file systems is serialized by the wait for SB_DEAD. > > Remove the superfluous sb_mutex. > > Signed-off-by: Christoph Hellwig [...] MBR, Sergey
[PATCH 2/8] usb: gadget: add anonymous definition in some struct for trace purpose
Some UDC trace event will save usb udc information, but it use one int
size buffer to save one bit information of usb udc, it is wast trace
buffer.
Add anonymous union which have one u32 member can be used by trace event
during fast assign stage to save more entries with same trace ring buffer
size.
Signed-off-by: Linyu Yuan
---
include/linux/usb/gadget.h | 113 +++--
1 file changed, 72 insertions(+), 41 deletions(-)
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 75bda0783395..4894f256df55 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -41,6 +41,7 @@ struct usb_ep;
* @num_sgs: number of SG entries
* @num_mapped_sgs: number of SG entries mapped to DMA (internal)
* @length: Length of that data
+ * @dw1: trace event purpose
* @stream_id: The stream id, when USB3.0 bulk streams are being used
* @is_last: Indicates if this is the last request of a stream_id before
* switching to a different stream (required for DWC3 controllers).
@@ -105,12 +106,17 @@ struct usb_request {
unsignednum_sgs;
unsignednum_mapped_sgs;
- unsignedstream_id:16;
- unsignedis_last:1;
- unsignedno_interrupt:1;
- unsignedzero:1;
- unsignedshort_not_ok:1;
- unsigneddma_mapped:1;
+ union {
+ struct {
+ u32 stream_id:16;
+ u32 is_last:1;
+ u32 no_interrupt:1;
+ u32 zero:1;
+ u32 short_not_ok:1;
+ u32 dma_mapped:1;
+ } __packed;
+ u32 dw1;
+ };
void(*complete)(struct usb_ep *ep,
struct usb_request *req);
@@ -163,13 +169,13 @@ struct usb_ep_ops {
* @dir_out:Endpoint supports OUT direction.
*/
struct usb_ep_caps {
- unsigned type_control:1;
- unsigned type_iso:1;
- unsigned type_bulk:1;
- unsigned type_int:1;
- unsigned dir_in:1;
- unsigned dir_out:1;
-};
+ u8 type_control:1;
+ u8 type_iso:1;
+ u8 type_bulk:1;
+ u8 type_int:1;
+ u8 dir_in:1;
+ u8 dir_out:1;
+} __packed;
#define USB_EP_CAPS_TYPE_CONTROL 0x01
#define USB_EP_CAPS_TYPE_ISO 0x02
@@ -199,6 +205,9 @@ struct usb_ep_caps {
* @caps:The structure describing types and directions supported by endpoint.
* @enabled: The current endpoint enabled/disabled state.
* @claimed: True if this endpoint is claimed by a function.
+ * @dw1: trace event purpose
+ * @dw2: trace event purpose
+ * @dw3: trace event purpose
* @maxpacket:The maximum packet size used on this endpoint. The initial
* value can sometimes be reduced (hardware allowing), according to
* the endpoint descriptor used to configure the endpoint.
@@ -228,15 +237,30 @@ struct usb_ep {
const char *name;
const struct usb_ep_ops *ops;
struct list_headep_list;
- struct usb_ep_caps caps;
- boolclaimed;
- boolenabled;
- unsignedmaxpacket:16;
- unsignedmaxpacket_limit:16;
- unsignedmax_streams:16;
- unsignedmult:2;
- unsignedmaxburst:5;
- u8 address;
+ union {
+ struct {
+ u32 maxpacket:16;
+ u32 maxpacket_limit:16;
+ } __packed;
+ u32 dw1;
+ };
+ union {
+ struct {
+ u32 max_streams:16;
+ u32 mult:2;
+ u32 maxburst:5;
+ } __packed;
+ u32 dw2;
+ };
+ union {
+ struct {
+ struct usb_ep_caps caps;
+ u8 claimed:1;
+ u8 enabled:1;
+ u8 address;
+ } __packed;
+ u32 dw3;
+ };
const struct usb_endpoint_descriptor*desc;
const struct usb_ss_ep_comp_descriptor *comp_desc;
};
@@ -357,6 +381,7 @@ struct usb_gadget_ops {
* @in_epnum: last used in ep number
* @mA: last set mA value
* @otg_caps: OTG capabilities of this gadget.
+ * @dw1: trace event purpose
* @sg_supported: true if we can handle scatter-gather
* @is_otg: True if the USB device port uses a Mini-AB jack, so that the
* gadget driver must provide a USB OTG descriptor.
@@ -432,25 +457,31 @@ struct usb_gadget {
unsignedmA;
[PATCH 8/8] usb: musb: trace: reduce buffer usage of trace event
Save u32 member into trace event ring buffer and parse it for possible
bit information.
Use DECLARE_EVENT_CLASS_PRINT_INIT() related macro for output stage.
Signed-off-by: Linyu Yuan
---
drivers/usb/musb/musb_trace.h | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/musb/musb_trace.h b/drivers/usb/musb/musb_trace.h
index f246b14394c4..8add5e81ed8d 100644
--- a/drivers/usb/musb/musb_trace.h
+++ b/drivers/usb/musb/musb_trace.h
@@ -233,7 +233,7 @@ DEFINE_EVENT(musb_urb, musb_urb_deq,
TP_ARGS(musb, urb)
);
-DECLARE_EVENT_CLASS(musb_req,
+DECLARE_EVENT_CLASS_PRINT_INIT(musb_req,
TP_PROTO(struct musb_request *req),
TP_ARGS(req),
TP_STRUCT__entry(
@@ -243,9 +243,7 @@ DECLARE_EVENT_CLASS(musb_req,
__field(int, status)
__field(unsigned int, buf_len)
__field(unsigned int, actual_len)
- __field(unsigned int, zero)
- __field(unsigned int, short_not_ok)
- __field(unsigned int, no_interrupt)
+ __field(u32, rdw1)
),
TP_fast_assign(
__entry->req = >request;
@@ -254,18 +252,20 @@ DECLARE_EVENT_CLASS(musb_req,
__entry->status = req->request.status;
__entry->buf_len = req->request.length;
__entry->actual_len = req->request.actual;
- __entry->zero = req->request.zero;
- __entry->short_not_ok = req->request.short_not_ok;
- __entry->no_interrupt = req->request.no_interrupt;
+ __entry->rdw1 = req->request.dw1;
),
TP_printk("%p, ep%d %s, %s%s%s, len %d/%d, status %d",
__entry->req, __entry->epnum,
__entry->is_tx ? "tx/IN" : "rx/OUT",
- __entry->zero ? "Z" : "z",
- __entry->short_not_ok ? "S" : "s",
- __entry->no_interrupt ? "I" : "i",
+ tr.zero ? "Z" : "z",
+ tr.short_not_ok ? "S" : "s",
+ tr.no_interrupt ? "I" : "i",
__entry->actual_len, __entry->buf_len,
__entry->status
+ ),
+ TP_printk_init(
+ struct usb_request tr;
+ tr.dw1 = __entry->rdw1;
)
);
--
2.17.1
[PATCH 7/8] usb: mtu3: trace: reduce buffer usage of trace event
Save u32 members into trace event ring buffer and parse it for possible
bit information.
Use DECLARE_EVENT_CLASS_PRINT_INIT() related macro for output stage.
Signed-off-by: Linyu Yuan
---
drivers/usb/mtu3/mtu3_trace.h | 76 +++
1 file changed, 50 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/mtu3/mtu3_trace.h b/drivers/usb/mtu3/mtu3_trace.h
index 03d2a9bac27e..22b821771c24 100644
--- a/drivers/usb/mtu3/mtu3_trace.h
+++ b/drivers/usb/mtu3/mtu3_trace.h
@@ -113,35 +113,43 @@ DEFINE_EVENT(mtu3_log_setup, mtu3_handle_setup,
TP_ARGS(setup)
);
-DECLARE_EVENT_CLASS(mtu3_log_request,
+DECLARE_EVENT_CLASS_PRINT_INIT(mtu3_log_request,
TP_PROTO(struct mtu3_request *mreq),
TP_ARGS(mreq),
TP_STRUCT__entry(
- __string(name, mreq->mep->name)
+ __field(u32, edw3)
__field(struct mtu3_request *, mreq)
__field(struct qmu_gpd *, gpd)
__field(unsigned int, actual)
__field(unsigned int, length)
__field(int, status)
- __field(int, zero)
- __field(int, no_interrupt)
+ __field(u32, rdw1)
),
TP_fast_assign(
- __assign_str(name, mreq->mep->name);
+ __entry->edw3 = mreq->mep->ep.dw3;
__entry->mreq = mreq;
__entry->gpd = mreq->gpd;
__entry->actual = mreq->request.actual;
__entry->length = mreq->request.length;
__entry->status = mreq->request.status;
- __entry->zero = mreq->request.zero;
- __entry->no_interrupt = mreq->request.no_interrupt;
+ __entry->rdw1 = mreq->request.dw1;
),
TP_printk("%s: req %p gpd %p len %u/%u %s%s --> %d",
- __get_str(name), __entry->mreq, __entry->gpd,
+ __s, __entry->mreq, __entry->gpd,
__entry->actual, __entry->length,
- __entry->zero ? "Z" : "z",
- __entry->no_interrupt ? "i" : "I",
+ tr.zero ? "Z" : "z",
+ tr.no_interrupt ? "i" : "I",
__entry->status
+ ),
+ TP_printk_init(
+ struct usb_ep te;
+ struct usb_request tr;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ tr.dw1 = __entry->rdw1;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
)
);
@@ -170,11 +178,11 @@ DEFINE_EVENT(mtu3_log_request, mtu3_req_complete,
TP_ARGS(req)
);
-DECLARE_EVENT_CLASS(mtu3_log_gpd,
+DECLARE_EVENT_CLASS_PRINT_INIT(mtu3_log_gpd,
TP_PROTO(struct mtu3_ep *mep, struct qmu_gpd *gpd),
TP_ARGS(mep, gpd),
TP_STRUCT__entry(
- __string(name, mep->name)
+ __field(u32, edw3)
__field(struct qmu_gpd *, gpd)
__field(u32, dw0)
__field(u32, dw1)
@@ -182,7 +190,7 @@ DECLARE_EVENT_CLASS(mtu3_log_gpd,
__field(u32, dw3)
),
TP_fast_assign(
- __assign_str(name, mep->name);
+ __entry->edw3 = mep->ep.dw3;
__entry->gpd = gpd;
__entry->dw0 = le32_to_cpu(gpd->dw0_info);
__entry->dw1 = le32_to_cpu(gpd->next_gpd);
@@ -190,9 +198,17 @@ DECLARE_EVENT_CLASS(mtu3_log_gpd,
__entry->dw3 = le32_to_cpu(gpd->dw3_info);
),
TP_printk("%s: gpd %p - %08x %08x %08x %08x",
- __get_str(name), __entry->gpd,
+ __s, __entry->gpd,
__entry->dw0, __entry->dw1,
__entry->dw2, __entry->dw3
+ ),
+ TP_printk_init(
+ struct usb_ep te;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
)
);
@@ -211,41 +227,49 @@ DEFINE_EVENT(mtu3_log_gpd, mtu3_zlp_exp_gpd,
TP_ARGS(mep, gpd)
);
-DECLARE_EVENT_CLASS(mtu3_log_ep,
+DECLARE_EVENT_CLASS_PRINT_INIT(mtu3_log_ep,
TP_PROTO(struct mtu3_ep *mep),
TP_ARGS(mep),
TP_STRUCT__entry(
- __string(name, mep->name)
+ __field(u32, edw3)
__field(unsigned int, type)
__field(unsigned int, slot)
- __field(unsigned int, maxp)
- __field(unsigned int, mult)
- __field(unsigned int, maxburst)
+ __field(u32, edw1)
+ __field(u32, edw2)
__field(unsigned int, flags)
__field(unsigned int, direction)
__field(struct mtu3_gpd_ring *, gpd_ring)
),
TP_fast_assign(
-
[PATCH 6/8] usb: cdns2: trace: reduce buffer usage of trace event
Save u32 members into trace event ring buffer and parse it for possible
bit information.
Use DECLARE_EVENT_CLASS_PRINT_INIT() related macro for output stage.
Signed-off-by: Linyu Yuan
---
drivers/usb/gadget/udc/cdns2/cdns2-trace.h | 175 ++---
1 file changed, 121 insertions(+), 54 deletions(-)
diff --git a/drivers/usb/gadget/udc/cdns2/cdns2-trace.h
b/drivers/usb/gadget/udc/cdns2/cdns2-trace.h
index 61f241634ea5..f81caa12cb63 100644
--- a/drivers/usb/gadget/udc/cdns2/cdns2-trace.h
+++ b/drivers/usb/gadget/udc/cdns2/cdns2-trace.h
@@ -94,51 +94,74 @@ DEFINE_EVENT(cdns2_log_simple, cdns2_device_state,
TP_ARGS(msg)
);
-TRACE_EVENT(cdns2_ep_halt,
+TRACE_EVENT_PRINT_INIT(cdns2_ep_halt,
TP_PROTO(struct cdns2_endpoint *ep_priv, u8 halt, u8 flush),
TP_ARGS(ep_priv, halt, flush),
TP_STRUCT__entry(
- __string(name, ep_priv->name)
+ __field(u32, edw3)
__field(u8, halt)
__field(u8, flush)
),
TP_fast_assign(
- __assign_str(name, ep_priv->name);
+ __entry->edw3 = ep_priv->endpoint.dw3;
__entry->halt = halt;
__entry->flush = flush;
),
TP_printk("Halt %s for %s: %s", __entry->flush ? " and flush" : "",
- __get_str(name), __entry->halt ? "set" : "cleared")
+ __s, __entry->halt ? "set" : "cleared"),
+ TP_printk_init(
+ struct usb_ep te;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
+ )
);
-TRACE_EVENT(cdns2_wa1,
+TRACE_EVENT_PRINT_INIT(cdns2_wa1,
TP_PROTO(struct cdns2_endpoint *ep_priv, char *msg),
TP_ARGS(ep_priv, msg),
TP_STRUCT__entry(
- __string(ep_name, ep_priv->name)
+ __field(u32, edw3)
__string(msg, msg)
),
TP_fast_assign(
- __assign_str(ep_name, ep_priv->name);
+ __entry->edw3 = ep_priv->endpoint.dw3;
__assign_str(msg, msg);
),
- TP_printk("WA1: %s %s", __get_str(ep_name), __get_str(msg))
+ TP_printk("WA1: %s %s", __s, __get_str(msg)),
+ TP_printk_init(
+ struct usb_ep te;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
+ )
);
-DECLARE_EVENT_CLASS(cdns2_log_doorbell,
+DECLARE_EVENT_CLASS_PRINT_INIT(cdns2_log_doorbell,
TP_PROTO(struct cdns2_endpoint *pep, u32 ep_trbaddr),
TP_ARGS(pep, ep_trbaddr),
TP_STRUCT__entry(
- __string(name, pep->num ? pep->name :
- (pep->dir ? "ep0in" : "ep0out"))
+ __field(u32, edw3)
__field(u32, ep_trbaddr)
),
TP_fast_assign(
- __assign_str(name, pep->name);
+ __entry->edw3 = pep->endpoint.dw3;
__entry->ep_trbaddr = ep_trbaddr;
),
- TP_printk("%s, ep_trbaddr %08x", __get_str(name),
- __entry->ep_trbaddr)
+ TP_printk("%s, ep_trbaddr %08x", __s,
+ __entry->ep_trbaddr),
+ TP_printk_init(
+ struct usb_ep te;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
+ )
);
DEFINE_EVENT(cdns2_log_doorbell, cdns2_doorbell_ep0,
@@ -186,26 +209,34 @@ TRACE_EVENT(cdns2_dma_ep_ists,
__entry->dma_ep_ists >> 16)
);
-DECLARE_EVENT_CLASS(cdns2_log_epx_irq,
+DECLARE_EVENT_CLASS_PRINT_INIT(cdns2_log_epx_irq,
TP_PROTO(struct cdns2_device *pdev, struct cdns2_endpoint *pep),
TP_ARGS(pdev, pep),
TP_STRUCT__entry(
- __string(ep_name, pep->name)
+ __field(u32, edw3)
__field(u32, ep_sts)
__field(u32, ep_ists)
__field(u32, ep_traddr)
),
TP_fast_assign(
- __assign_str(ep_name, pep->name);
+ __entry->edw3 = pep->endpoint.dw3;
__entry->ep_sts = readl(>adma_regs->ep_sts);
__entry->ep_ists = readl(>adma_regs->ep_ists);
__entry->ep_traddr = readl(>adma_regs->ep_traddr);
),
TP_printk("%s, ep_traddr: %08x",
cdns2_decode_epx_irq(__get_buf(CDNS2_MSG_MAX), CDNS2_MSG_MAX,
- __get_str(ep_name),
+ __s,
[PATCH 5/8] usb: dwc3: trace: reduce buffer usage of trace event
Save u32 members into trace event ring buffer and parse it for possible
bit information.
Use DECLARE_EVENT_CLASS_PRINT_INIT() related macro for output stage.
Signed-off-by: Linyu Yuan
---
drivers/usb/dwc3/trace.h | 99 +---
1 file changed, 63 insertions(+), 36 deletions(-)
diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index d2997d17cfbe..3caac180b225 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -98,35 +98,41 @@ DEFINE_EVENT(dwc3_log_ctrl, dwc3_ctrl_req,
TP_ARGS(ctrl)
);
-DECLARE_EVENT_CLASS(dwc3_log_request,
+DECLARE_EVENT_CLASS_PRINT_INIT(dwc3_log_request,
TP_PROTO(struct dwc3_request *req),
TP_ARGS(req),
TP_STRUCT__entry(
- __string(name, req->dep->name)
+ __field(u32, edw3)
__field(struct dwc3_request *, req)
__field(unsigned int, actual)
__field(unsigned int, length)
__field(int, status)
- __field(int, zero)
- __field(int, short_not_ok)
- __field(int, no_interrupt)
+ __field(u32, rdw1)
),
TP_fast_assign(
- __assign_str(name, req->dep->name);
+ __entry->edw3 = req->dep->endpoint.dw3;
__entry->req = req;
__entry->actual = req->request.actual;
__entry->length = req->request.length;
__entry->status = req->request.status;
- __entry->zero = req->request.zero;
- __entry->short_not_ok = req->request.short_not_ok;
- __entry->no_interrupt = req->request.no_interrupt;
+ __entry->rdw1 = req->request.dw1;
),
TP_printk("%s: req %p length %u/%u %s%s%s ==> %d",
- __get_str(name), __entry->req, __entry->actual, __entry->length,
- __entry->zero ? "Z" : "z",
- __entry->short_not_ok ? "S" : "s",
- __entry->no_interrupt ? "i" : "I",
+ __s, __entry->req, __entry->actual, __entry->length,
+ tr.zero ? "Z" : "z",
+ tr.short_not_ok ? "S" : "s",
+ tr.no_interrupt ? "i" : "I",
__entry->status
+ ),
+ TP_printk_init(
+ struct usb_ep te;
+ struct usb_request tr;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ tr.dw1 = __entry->rdw1;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
)
);
@@ -180,12 +186,12 @@ DEFINE_EVENT(dwc3_log_generic_cmd,
dwc3_gadget_generic_cmd,
TP_ARGS(cmd, param, status)
);
-DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
+DECLARE_EVENT_CLASS_PRINT_INIT(dwc3_log_gadget_ep_cmd,
TP_PROTO(struct dwc3_ep *dep, unsigned int cmd,
struct dwc3_gadget_ep_cmd_params *params, int cmd_status),
TP_ARGS(dep, cmd, params, cmd_status),
TP_STRUCT__entry(
- __string(name, dep->name)
+ __field(u32, edw3)
__field(unsigned int, cmd)
__field(u32, param0)
__field(u32, param1)
@@ -193,7 +199,7 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
__field(int, cmd_status)
),
TP_fast_assign(
- __assign_str(name, dep->name);
+ __entry->edw3 = dep->endpoint.dw3;
__entry->cmd = cmd;
__entry->param0 = params->param0;
__entry->param1 = params->param1;
@@ -201,10 +207,18 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
__entry->cmd_status = cmd_status;
),
TP_printk("%s: cmd '%s' [%x] params %08x %08x %08x --> status: %s",
- __get_str(name), dwc3_gadget_ep_cmd_string(__entry->cmd),
+ __s, dwc3_gadget_ep_cmd_string(__entry->cmd),
__entry->cmd, __entry->param0,
__entry->param1, __entry->param2,
dwc3_ep_cmd_status_string(__entry->cmd_status)
+ ),
+ TP_printk_init(
+ struct usb_ep te;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
)
);
@@ -214,11 +228,11 @@ DEFINE_EVENT(dwc3_log_gadget_ep_cmd, dwc3_gadget_ep_cmd,
TP_ARGS(dep, cmd, params, cmd_status)
);
-DECLARE_EVENT_CLASS(dwc3_log_trb,
+DECLARE_EVENT_CLASS_PRINT_INIT(dwc3_log_trb,
TP_PROTO(struct dwc3_ep *dep, struct dwc3_trb *trb),
TP_ARGS(dep, trb),
TP_STRUCT__entry(
- __string(name, dep->name)
+ __field(u32, edw3)
__field(struct dwc3_trb *, trb)
[PATCH 4/8] usb: cdns3: trace: reduce buffer usage of trace event
Save u32 members into trace event ring buffer and parse it for possible
bit information.
Use DECLARE_EVENT_CLASS_PRINT_INIT() related macro for output stage.
Signed-off-by: Linyu Yuan
---
drivers/usb/cdns3/cdns3-trace.h | 201 ++--
drivers/usb/cdns3/cdnsp-trace.h | 105 +++--
2 files changed, 209 insertions(+), 97 deletions(-)
diff --git a/drivers/usb/cdns3/cdns3-trace.h b/drivers/usb/cdns3/cdns3-trace.h
index 40db89ec..9711d4b48eb6 100644
--- a/drivers/usb/cdns3/cdns3-trace.h
+++ b/drivers/usb/cdns3/cdns3-trace.h
@@ -24,49 +24,73 @@
#define CDNS3_MSG_MAX 500
-TRACE_EVENT(cdns3_halt,
+TRACE_EVENT_PRINT_INIT(cdns3_halt,
TP_PROTO(struct cdns3_endpoint *ep_priv, u8 halt, u8 flush),
TP_ARGS(ep_priv, halt, flush),
TP_STRUCT__entry(
- __string(name, ep_priv->name)
+ __field(u32, edw3)
__field(u8, halt)
__field(u8, flush)
),
TP_fast_assign(
- __assign_str(name, ep_priv->name);
+ __entry->edw3 = ep_priv->endpoint.dw3;
__entry->halt = halt;
__entry->flush = flush;
),
TP_printk("Halt %s for %s: %s", __entry->flush ? " and flush" : "",
- __get_str(name), __entry->halt ? "set" : "cleared")
+ __s, __entry->halt ? "set" : "cleared"),
+ TP_printk_init(
+ struct usb_ep te;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
+ )
);
-TRACE_EVENT(cdns3_wa1,
+TRACE_EVENT_PRINT_INIT(cdns3_wa1,
TP_PROTO(struct cdns3_endpoint *ep_priv, char *msg),
TP_ARGS(ep_priv, msg),
TP_STRUCT__entry(
- __string(ep_name, ep_priv->name)
+ __field(u32, edw3)
__string(msg, msg)
),
TP_fast_assign(
- __assign_str(ep_name, ep_priv->name);
+ __entry->edw3 = ep_priv->endpoint.dw3;
__assign_str(msg, msg);
),
- TP_printk("WA1: %s %s", __get_str(ep_name), __get_str(msg))
+ TP_printk("WA1: %s %s", __s, __get_str(msg)),
+ TP_printk_init(
+ struct usb_ep te;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
+ )
);
-TRACE_EVENT(cdns3_wa2,
+TRACE_EVENT_PRINT_INIT(cdns3_wa2,
TP_PROTO(struct cdns3_endpoint *ep_priv, char *msg),
TP_ARGS(ep_priv, msg),
TP_STRUCT__entry(
- __string(ep_name, ep_priv->name)
+ __field(u32, edw3)
__string(msg, msg)
),
TP_fast_assign(
- __assign_str(ep_name, ep_priv->name);
+ __entry->edw3 = ep_priv->endpoint.dw3;
__assign_str(msg, msg);
),
- TP_printk("WA2: %s %s", __get_str(ep_name), __get_str(msg))
+ TP_printk("WA2: %s %s", __s, __get_str(msg)),
+ TP_printk_init(
+ struct usb_ep te;
+ char __s[9];
+ te.dw3 = __entry->edw3;
+ snprintf(__s, 9, "ep%d%s", te.address, \
+ (te.caps.dir_in && te.caps.dir_out) ? "" : \
+ te.caps.dir_in ? "in" : "out");
+ )
);
DECLARE_EVENT_CLASS(cdns3_log_doorbell,
@@ -114,18 +138,18 @@ DEFINE_EVENT(cdns3_log_usb_irq, cdns3_usb_irq,
TP_ARGS(priv_dev, usb_ists)
);
-DECLARE_EVENT_CLASS(cdns3_log_epx_irq,
+DECLARE_EVENT_CLASS_PRINT_INIT(cdns3_log_epx_irq,
TP_PROTO(struct cdns3_device *priv_dev, struct cdns3_endpoint *priv_ep),
TP_ARGS(priv_dev, priv_ep),
TP_STRUCT__entry(
- __string(ep_name, priv_ep->name)
+ __field(u32, edw3)
__field(u32, ep_sts)
__field(u32, ep_traddr)
__field(u32, ep_last_sid)
__field(u32, use_streams)
),
TP_fast_assign(
- __assign_str(ep_name, priv_ep->name);
+ __entry->edw3 = priv_ep->endpoint.dw3;
__entry->ep_sts = readl(_dev->regs->ep_sts);
__entry->ep_traddr = readl(_dev->regs->ep_traddr);
__entry->ep_last_sid = priv_ep->last_stream_id;
@@ -133,11 +157,19 @@ DECLARE_EVENT_CLASS(cdns3_log_epx_irq,
),
TP_printk("%s, ep_traddr: %08x ep_last_sid: %08x use_streams: %d",
cdns3_decode_epx_irq(__get_buf(CDNS3_MSG_MAX),
- __get_str(ep_name),
+ __s,
__entry->ep_sts),
__entry->ep_traddr,
[PATCH 3/8] usb: udc: trace: reduce buffer usage of trace event
Save u32 members into trace event ring buffer and parse it for possible
bit fields.
Use new DECLARE_EVENT_CLASS_PRINT_INIT() class macro for output stage.
Signed-off-by: Linyu Yuan
---
drivers/usb/gadget/udc/trace.h | 154 +++--
1 file changed, 69 insertions(+), 85 deletions(-)
diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
index a5ed26fbc2da..e1754667f1d2 100644
--- a/drivers/usb/gadget/udc/trace.h
+++ b/drivers/usb/gadget/udc/trace.h
@@ -17,7 +17,7 @@
#include
#include
-DECLARE_EVENT_CLASS(udc_log_gadget,
+DECLARE_EVENT_CLASS_PRINT_INIT(udc_log_gadget,
TP_PROTO(struct usb_gadget *g, int ret),
TP_ARGS(g, ret),
TP_STRUCT__entry(
@@ -25,20 +25,7 @@ DECLARE_EVENT_CLASS(udc_log_gadget,
__field(enum usb_device_speed, max_speed)
__field(enum usb_device_state, state)
__field(unsigned, mA)
- __field(unsigned, sg_supported)
- __field(unsigned, is_otg)
- __field(unsigned, is_a_peripheral)
- __field(unsigned, b_hnp_enable)
- __field(unsigned, a_hnp_support)
- __field(unsigned, hnp_polling_support)
- __field(unsigned, host_request_flag)
- __field(unsigned, quirk_ep_out_aligned_size)
- __field(unsigned, quirk_altset_not_supp)
- __field(unsigned, quirk_stall_not_supp)
- __field(unsigned, quirk_zlp_not_supp)
- __field(unsigned, is_selfpowered)
- __field(unsigned, deactivated)
- __field(unsigned, connected)
+ __field(u32, gdw1)
__field(int, ret)
),
TP_fast_assign(
@@ -46,39 +33,35 @@ DECLARE_EVENT_CLASS(udc_log_gadget,
__entry->max_speed = g->max_speed;
__entry->state = g->state;
__entry->mA = g->mA;
- __entry->sg_supported = g->sg_supported;
- __entry->is_otg = g->is_otg;
- __entry->is_a_peripheral = g->is_a_peripheral;
- __entry->b_hnp_enable = g->b_hnp_enable;
- __entry->a_hnp_support = g->a_hnp_support;
- __entry->hnp_polling_support = g->hnp_polling_support;
- __entry->host_request_flag = g->host_request_flag;
- __entry->quirk_ep_out_aligned_size =
g->quirk_ep_out_aligned_size;
- __entry->quirk_altset_not_supp = g->quirk_altset_not_supp;
- __entry->quirk_stall_not_supp = g->quirk_stall_not_supp;
- __entry->quirk_zlp_not_supp = g->quirk_zlp_not_supp;
- __entry->is_selfpowered = g->is_selfpowered;
- __entry->deactivated = g->deactivated;
- __entry->connected = g->connected;
+ __entry->gdw1 = g->dw1;
__entry->ret = ret;
),
- TP_printk("speed %d/%d state %d %dmA [%s%s%s%s%s%s%s%s%s%s%s%s%s%s] -->
%d",
+ TP_printk("speed %d/%d state %d %dmA
[%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s] --> %d",
__entry->speed, __entry->max_speed, __entry->state, __entry->mA,
- __entry->sg_supported ? "sg:" : "",
- __entry->is_otg ? "OTG:" : "",
- __entry->is_a_peripheral ? "a_peripheral:" : "",
- __entry->b_hnp_enable ? "b_hnp:" : "",
- __entry->a_hnp_support ? "a_hnp:" : "",
- __entry->hnp_polling_support ? "hnp_poll:" : "",
- __entry->host_request_flag ? "hostreq:" : "",
- __entry->quirk_ep_out_aligned_size ? "out_aligned:" : "",
- __entry->quirk_altset_not_supp ? "no_altset:" : "",
- __entry->quirk_stall_not_supp ? "no_stall:" : "",
- __entry->quirk_zlp_not_supp ? "no_zlp" : "",
- __entry->is_selfpowered ? "self-powered:" : "bus-powered:",
- __entry->deactivated ? "deactivated:" : "activated:",
- __entry->connected ? "connected" : "disconnected",
- __entry->ret)
+ tg.sg_supported ? "sg:" : "",
+ tg.is_otg ? "OTG:" : "",
+ tg.is_a_peripheral ? "a_peripheral:" : "",
+ tg.b_hnp_enable ? "b_hnp:" : "",
+ tg.a_hnp_support ? "a_hnp:" : "",
+ tg.a_alt_hnp_support ? "a_alt_hnp:" : "",
+ tg.hnp_polling_support ? "hnp_poll:" : "",
+ tg.host_request_flag ? "hostreq:" : "",
+ tg.quirk_ep_out_aligned_size ? "out_aligned:" : "",
+ tg.quirk_altset_not_supp ? "no_altset:" : "",
+ tg.quirk_stall_not_supp ? "no_stall:" : "",
+ tg.quirk_zlp_not_supp ? "no_zlp" : "",
+ tg.quirk_avoids_skb_reserve ? "no_skb_reserve" : "",
+ tg.is_selfpowered ? "self-powered:" : "bus-powered:",
+ tg.deactivated ? "deactivated:" : "activated:",
+ tg.connected ?
[PATCH 0/8] usb: gadget: reduce usb gadget trace event buffer usage
some trace event use an interger to to save a bit field info of gadget, also some trace save endpoint name in string forat, it all can be chagned to other way at trace event store phase. bit field can be replace with a union interger member which include multiple bit fields. ep name stringe can be replace to a interger which contaion number and dir info. to allow trace output stage can get bit info from save interger, add DECLARE_EVENT_CLASS_PRINT_INIT() clas which allow user defined operation before print. v1: https://lore.kernel.org/linux-usb/20230911042843.2711-1-quic_linyy...@quicinc.com/ v2: fix two compile issues that COMPILE_TEST not covered https://lore.kernel.org/linux-usb/2023092446.1791-1-quic_linyy...@quicinc.com/ v3: fix reviewer comments, allow bit fields work on both little and big endian https://lore.kernel.org/linux-usb/20230912104455.7737-1-quic_linyy...@quicinc.com/ v4: add DECLARE_EVENT_CLASS_PRINT_INIT() new trace class and use it Linyu Yuan (8): trace: add new DECLARE_EVENT_CLASS_PRINT_INIT class type usb: gadget: add anonymous definition in some struct for trace purpose usb: udc: trace: reduce buffer usage of trace event usb: cdns3: trace: reduce buffer usage of trace event usb: dwc3: trace: reduce buffer usage of trace event usb: cdns2: trace: reduce buffer usage of trace event usb: mtu3: trace: reduce buffer usage of trace event usb: musb: trace: reduce buffer usage of trace event drivers/usb/cdns3/cdns3-trace.h| 201 ++--- drivers/usb/cdns3/cdnsp-trace.h| 105 +++ drivers/usb/dwc3/trace.h | 99 ++ drivers/usb/gadget/udc/cdns2/cdns2-trace.h | 175 -- drivers/usb/gadget/udc/trace.h | 154 +++- drivers/usb/mtu3/mtu3_trace.h | 76 +--- drivers/usb/musb/musb_trace.h | 20 +- include/linux/tracepoint.h | 22 +++ include/linux/usb/gadget.h | 113 +++- include/trace/bpf_probe.h | 4 + include/trace/perf.h | 43 + include/trace/stages/stage3_trace_output.h | 3 + include/trace/trace_events.h | 118 13 files changed, 784 insertions(+), 349 deletions(-) -- 2.17.1
[PATCH 1/8] trace: add new DECLARE_EVENT_CLASS_PRINT_INIT class type
This class almost same as DECLARE_EVENT_CLASS, it allow user add some
init operation before print at output stage.
Add a new macro TP_printk_init(), user can add operation in it.
Also add a new TRACE_EVENT_PRINT_INIT() macro.
Signed-off-by: Linyu Yuan
---
include/linux/tracepoint.h | 22
include/trace/bpf_probe.h | 4 +
include/trace/perf.h | 43
include/trace/stages/stage3_trace_output.h | 3 +
include/trace/trace_events.h | 118 +
5 files changed, 190 insertions(+)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 88c0ba623ee6..3fd42640236a 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -551,6 +551,7 @@ static inline struct tracepoint
*tracepoint_ptr_deref(tracepoint_ptr_t *p)
*/
#define DECLARE_EVENT_CLASS(name, proto, args, tstruct, assign, print)
+#define DECLARE_EVENT_CLASS_PRINT_INIT(name, proto, args, tstruct, assign,
print, init)
#define DEFINE_EVENT(template, name, proto, args) \
DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
#define DEFINE_EVENT_FN(template, name, proto, args, reg, unreg)\
@@ -576,6 +577,20 @@ static inline struct tracepoint
*tracepoint_ptr_deref(tracepoint_ptr_t *p)
DECLARE_TRACE_CONDITION(name, PARAMS(proto),\
PARAMS(args), PARAMS(cond))
+#define TRACE_EVENT_PRINT_INIT(name, proto, args, struct, assign, print, init)
\
+ DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
+#define TRACE_EVENT_FN_PRINT_INIT(name, proto, args, struct, \
+ assign, print, reg, unreg, init)\
+ DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
+#define TRACE_EVENT_FN_COND_PRINT_INIT(name, proto, args, cond, struct,
\
+ assign, print, reg, unreg, init)\
+ DECLARE_TRACE_CONDITION(name, PARAMS(proto),\
+ PARAMS(args), PARAMS(cond))
+#define TRACE_EVENT_CONDITION_PRINT_INIT(name, proto, args, cond,
\
+ struct, assign, print, init) \
+ DECLARE_TRACE_CONDITION(name, PARAMS(proto),\
+ PARAMS(args), PARAMS(cond))
+
#define TRACE_EVENT_FLAGS(event, flag)
#define TRACE_EVENT_PERF_PERM(event, expr...)
@@ -595,4 +610,11 @@ static inline struct tracepoint
*tracepoint_ptr_deref(tracepoint_ptr_t *p)
#define DEFINE_EVENT_NOP(template, name, proto, args) \
DECLARE_EVENT_NOP(name, PARAMS(proto), PARAMS(args))
+#define TRACE_EVENT_NOP_PRINT_INIT(name, proto, args, struct, assign, print,
init) \
+ DECLARE_EVENT_NOP(name, PARAMS(proto), PARAMS(args))
+
+#define DECLARE_EVENT_CLASS_NOP_PRINT_INIT(name, proto, args, tstruct, assign,
print, init)
+#define DEFINE_EVENT_NOP(template, name, proto, args) \
+ DECLARE_EVENT_NOP(name, PARAMS(proto), PARAMS(args))
+
#endif /* ifdef TRACE_EVENT (see note above) */
diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
index e609cd7da47e..99b5594a4a8e 100644
--- a/include/trace/bpf_probe.h
+++ b/include/trace/bpf_probe.h
@@ -54,6 +54,10 @@ __bpf_trace_##call(void *__data, proto)
\
#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
__BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args))
+#undef DECLARE_EVENT_CLASS_PRINT_INIT
+#define DECLARE_EVENT_CLASS_PRINT_INIT(call, proto, args, tstruct, assign,
print, init)\
+ __BPF_DECLARE_TRACE(call, PARAMS(proto), PARAMS(args))
+
/*
* This part is compiled out, it is only here as a build time check
* to make sure that if the tracepoint handling changes, the
diff --git a/include/trace/perf.h b/include/trace/perf.h
index 2c11181c82e0..bee78e8eef5d 100644
--- a/include/trace/perf.h
+++ b/include/trace/perf.h
@@ -55,6 +55,49 @@ perf_trace_##call(void *__data, proto)
\
head, __task);\
}
+#undef DECLARE_EVENT_CLASS_PRINT_INIT
+#define DECLARE_EVENT_CLASS_PRINT_INIT(call, proto, args, tstruct, assign,
print, init)\
+static notrace void\
+perf_trace_##call(void *__data, proto) \
+{ \
+ struct trace_event_call *event_call = __data; \
+ struct trace_event_data_offsets_##call __maybe_unused __data_offsets;\
+ struct trace_event_raw_##call *entry; \
+ struct pt_regs *__regs; \
+ u64 __count = 1;\
+ struct task_struct *__task = NULL; \
+
Re: [PATCH 03/19] fs: release anon dev_t in deactivate_locked_super
> BTW, this part of commit message in 2c18a63b760a is rather confused: > Recent rework moved block device closing out of sb->put_super() and into > sb->kill_sb() to avoid deadlocks as s_umount is held in put_super() and > blkdev_put() can end up taking s_umount again. > > That was *NOT* what a recent rework had done. Block device closing had never > been inside ->put_super() - at no point since that (closing, that is) had been > introduced back in 0.97 ;-) ->put_super() predates it (0.95c+). I think the commit message probably just isn't clear enough. The main block device of a superblock isn't closed in sb->put_super(). That's always been closed in kill_block_super() after generic_shutdown_super(). But afaict filesystem like ext4 and xfs may have additional block devices open exclusively and closed them in sb->put_super(): xfs_fs_put_super() -> xfs_close_devices() -> xfs_blkdev_put() -> blkdev_put() ext4_put_super() -> ext4_blkdev_remove() -> blkdev_put()
Re: [PATCH v2 06/14] arm64: dts: qcom: sdm630: Drop RPM bus clocks
On 14.09.2023 08:26, Krzysztof Kozlowski wrote:
> On 13/09/2023 14:08, Konrad Dybcio wrote:
>> On 13.09.2023 09:13, Krzysztof Kozlowski wrote:
>>> On 12/09/2023 15:31, Konrad Dybcio wrote:
These clocks are now handled from within the icc framework and are
no longer registered from within the CCF. Remove them.
Signed-off-by: Konrad Dybcio
---
>> [...]
>>
anoc2_smmu: iommu@16c {
compatible = "qcom,sdm630-smmu-v2", "qcom,smmu-v2";
reg = <0x016c 0x4>;
-
- assigned-clocks = < RPM_SMD_AGGR2_NOC_CLK>;
- assigned-clock-rates = <1000>;
- clocks = < RPM_SMD_AGGR2_NOC_CLK>;
- clock-names = "bus";
>>>
>>> This is also against bindings. After your patch #4, such bus clock (or
>>> other combinations) is still required.
>> So, we have 4 SMMU instances on this platform:
>>
>> MMSS (described, iface, mem, mem_iface)
>> GPU (described, iface-mm, iface-smmu, bus-smmu)
>>
>> ANOC2 (this one, no clocks after removing rpmcc bus)
>> LPASS (no clocks)
>
> Ah, I did not notice it.
>
>>
>> Should I then create a new entry in the bindings, replicating
>> what's there for msm8998[1] and dropping the entry with just "bus"
>> from anyOf?
>
> So this passes the bindings, right?
Yes
anyOf: in the binding should allow
> also no match, so this should be fine. However indeed we need to drop
> the "bus" entry, because it is not valid anymore.
Actually, looks like the LPASS smmu may require a single
clock. We can reuse that single-"bus"-clock entry for
HLOS1_VOTE_LPASS_ADSP_SMMU_CLK.
The device didn't crash when trying to access LPASS SMMU
with that clock absent, but I guess it may have just
been luck, things may change once more hardware is parked..
Konrad
[PATCH] livepatch: Fix missing newline character in klp_resolve_symbols()
Without the newline character, the log may not be printed immediately
after the error occurs.
Fixes: ca376a937486 ("livepatch: Prevent module-specific KLP rela sections from
referencing vmlinux symbols")
Signed-off-by: Zheng Yejian
---
kernel/livepatch/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 61328328c474..ecbc9b6aba3a 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -243,7 +243,7 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const
char *strtab,
* symbols are exported and normal relas can be used instead.
*/
if (!sec_vmlinux && sym_vmlinux) {
- pr_err("invalid access to vmlinux symbol '%s' from
module-specific livepatch relocation section",
+ pr_err("invalid access to vmlinux symbol '%s' from
module-specific livepatch relocation section\n",
sym_name);
return -EINVAL;
}
--
2.25.1
[PATCH v3] libnvdimm/of_pmem: Use devm_kstrdup instead of kstrdup and check its return value
Use devm_kstrdup() instead of kstrdup() and check its return value to
avoid memory leak.
Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus
provider")
Signed-off-by: Chen Ni
---
Changelog:
v2 -> v3:
1. Use devm_kstrdup() instead of kstrdup()
v1 -> v2:
1. Add a fixes tag.
2. Update commit message.
---
drivers/nvdimm/of_pmem.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
index 1b9f5b8a6167..5765674b36f2 100644
--- a/drivers/nvdimm/of_pmem.c
+++ b/drivers/nvdimm/of_pmem.c
@@ -30,7 +30,13 @@ static int of_pmem_region_probe(struct platform_device *pdev)
if (!priv)
return -ENOMEM;
- priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
+ priv->bus_desc.provider_name = devm_kstrdup(>dev, pdev->name,
+ GFP_KERNEL);
+ if (!priv->bus_desc.provider_name) {
+ kfree(priv);
+ return -ENOMEM;
+ }
+
priv->bus_desc.module = THIS_MODULE;
priv->bus_desc.of_node = np;
--
2.25.1
Re: [PATCH v3] x86/platform/uv: refactor deprecated strcpy and strncpy
* Hans de Goede wrote: > >> Which IMHO is much more readable then what has landed now. But since > >> v2 has already landed I guess the best thing is just to stick with > >> what we have upstream now... > > > > Well, how about we do a delta patch with all the changes you suggested? > > I'm all for readability. > > So I started doing this and notices that all the string manipulation + > parsing done here is really just a DYI implementation of > sysfs_match_string(). > > So I have prepared a patch to switch to sysfs_match_string(), which > completely removes the need to make a copy of the val string. > > I'll submit the patch right after this email. Thank you - that looks a far more thorough cleanup indeed. Ingo
Re: [PATCH v2 06/14] arm64: dts: qcom: sdm630: Drop RPM bus clocks
On 13/09/2023 14:08, Konrad Dybcio wrote:
> On 13.09.2023 09:13, Krzysztof Kozlowski wrote:
>> On 12/09/2023 15:31, Konrad Dybcio wrote:
>>> These clocks are now handled from within the icc framework and are
>>> no longer registered from within the CCF. Remove them.
>>>
>>> Signed-off-by: Konrad Dybcio
>>> ---
> [...]
>
>>> anoc2_smmu: iommu@16c {
>>> compatible = "qcom,sdm630-smmu-v2", "qcom,smmu-v2";
>>> reg = <0x016c 0x4>;
>>> -
>>> - assigned-clocks = < RPM_SMD_AGGR2_NOC_CLK>;
>>> - assigned-clock-rates = <1000>;
>>> - clocks = < RPM_SMD_AGGR2_NOC_CLK>;
>>> - clock-names = "bus";
>>
>> This is also against bindings. After your patch #4, such bus clock (or
>> other combinations) is still required.
> So, we have 4 SMMU instances on this platform:
>
> MMSS (described, iface, mem, mem_iface)
> GPU (described, iface-mm, iface-smmu, bus-smmu)
>
> ANOC2 (this one, no clocks after removing rpmcc bus)
> LPASS (no clocks)
Ah, I did not notice it.
>
> Should I then create a new entry in the bindings, replicating
> what's there for msm8998[1] and dropping the entry with just "bus"
> from anyOf?
So this passes the bindings, right? anyOf: in the binding should allow
also no match, so this should be fine. However indeed we need to drop
the "bus" entry, because it is not valid anymore.
Best regards,
Krzysztof
