Re: [PATCH 09/12] mm/khugepaged: retract_page_tables() without mmap or vma lock
Thanks for looking, Peter: I was well aware of you dropping several hints
that you wanted to see what's intended before passing judgment on earlier
series, and I preferred to get on with showing this series, than go into
detail in responses to you there - thanks for your patience :)
On Mon, 29 May 2023, Peter Xu wrote:
> On Sun, May 28, 2023 at 11:25:15PM -0700, Hugh Dickins wrote:
...
> > @@ -1748,123 +1747,73 @@ static void
> > khugepaged_collapse_pte_mapped_thps(struct khugepaged_mm_slot *mm_sl
> > mmap_write_unlock(mm);
> > }
> >
> > -static int retract_page_tables(struct address_space *mapping, pgoff_t
> > pgoff,
> > - struct mm_struct *target_mm,
> > - unsigned long target_addr, struct page *hpage,
> > - struct collapse_control *cc)
> > +static void retract_page_tables(struct address_space *mapping, pgoff_t
> > pgoff)
> > {
> > struct vm_area_struct *vma;
> > - int target_result = SCAN_FAIL;
> >
> > - i_mmap_lock_write(mapping);
> > + i_mmap_lock_read(mapping);
> > vma_interval_tree_foreach(vma, >i_mmap, pgoff, pgoff) {
> > - int result = SCAN_FAIL;
> > - struct mm_struct *mm = NULL;
> > - unsigned long addr = 0;
> > - pmd_t *pmd;
> > - bool is_target = false;
> > + struct mm_struct *mm;
> > + unsigned long addr;
> > + pmd_t *pmd, pgt_pmd;
> > + spinlock_t *pml;
> > + spinlock_t *ptl;
> >
> > /*
> > * Check vma->anon_vma to exclude MAP_PRIVATE mappings that
> > -* got written to. These VMAs are likely not worth investing
> > -* mmap_write_lock(mm) as PMD-mapping is likely to be split
> > -* later.
> > +* got written to. These VMAs are likely not worth removing
> > +* page tables from, as PMD-mapping is likely to be split later.
> > *
> > -* Note that vma->anon_vma check is racy: it can be set up after
> > -* the check but before we took mmap_lock by the fault path.
> > -* But page lock would prevent establishing any new ptes of the
> > -* page, so we are safe.
> > -*
> > -* An alternative would be drop the check, but check that page
> > -* table is clear before calling pmdp_collapse_flush() under
> > -* ptl. It has higher chance to recover THP for the VMA, but
> > -* has higher cost too. It would also probably require locking
> > -* the anon_vma.
> > +* Note that vma->anon_vma check is racy: it can be set after
> > +* the check, but page locks (with XA_RETRY_ENTRYs in holes)
> > +* prevented establishing new ptes of the page. So we are safe
> > +* to remove page table below, without even checking it's empty.
> > */
> > - if (READ_ONCE(vma->anon_vma)) {
> > - result = SCAN_PAGE_ANON;
> > - goto next;
> > - }
> > + if (READ_ONCE(vma->anon_vma))
> > + continue;
>
> Not directly related to current patch, but I just realized there seems to
> have similar issue as what ab0c3f1251b4 wanted to fix.
>
> IIUC any shmem vma that used to have uprobe/bp installed will have anon_vma
> set here, then does it mean that any vma used to get debugged will never be
> able to merge into a thp (with either madvise or khugepaged)?
>
> I think it'll only make a difference when the page cache is not huge yet
> when bp was uninstalled, but then it becomes a thp candidate somehow. Even
> if so, I think the anon_vma should still be there.
>
> Did I miss something, or maybe that's not even a problem?
Finding vma->anon_vma set would discourage retract_page_tables() from
doing its business with that previously uprobed area; but it does not stop
collapse_pte_mapped_thp() (which uprobes unregister calls directly) from
dealing with it, and MADV_COLLAPSE works on anon_vma'ed areas too. It's
just a heuristic in retract_page_tables(), when it chooses to skip the
anon_vma'ed areas as often not worth bothering with.
As to vma merges: I haven't actually checked since the maple tree and other
rewrites of vma merging, but previously one vma with anon_vma set could be
merged with adjacent vma before or after without anon_vma set - the
anon_vma comparison is not just equality of anon_vma, but allows NULL too -
so the anon_vma will still be there, but extends to cover the wider extent.
Right, I find is_mergeable_anon_vma() still following that rule.
(And once vmas are merged, so that the whole of the huge page falls within
a single vma, khugepaged can consider it, and do collapse_pte_mapped_thp()
on it - before or after 11/12 I think.)
As to whether it would even be a problem: generally no, the vma is supposed
just to be an internal representation, and so long as the code
RE: [PATCH 0/6] bus: fsl-mc: Make remove function return void
> -Original Message- > From: Uwe Kleine-König > Sent: Tuesday, May 30, 2023 8:51 AM > To: Leo Li > Cc: Stuart Yoder ; Gaurav Jain > ; Roy Pledge ; Diana > Madalina Craciun (OSS) ; Eric Dumazet > ; Ioana Ciornei ; > k...@vger.kernel.org; Horia Geanta ; Jakub > Kicinski ; Paolo Abeni ; Laurentiu > Tudor ; Richard Cochran > ; Pankaj Gupta ; Alex > Williamson ; linux-arm- > ker...@lists.infradead.org; Herbert Xu ; > net...@vger.kernel.org; linux-ker...@vger.kernel.org; Vinod Koul > ; linux-cry...@vger.kernel.org; ker...@pengutronix.de; > Y.B. Lu ; dmaeng...@vger.kernel.org; linuxppc- > d...@lists.ozlabs.org; David S. Miller > Subject: Re: [PATCH 0/6] bus: fsl-mc: Make remove function return void > > Hello, > > On Mon, May 08, 2023 at 04:57:00PM -0500, Li Yang wrote: > > On Mon, May 8, 2023 at 8:44 AM Uwe Kleine-König > > wrote: > > > On Thu, Apr 13, 2023 at 08:00:04AM +0200, Uwe Kleine-König wrote: > > > > On Wed, Apr 12, 2023 at 09:30:05PM +, Leo Li wrote: > > > > > > On Fri, Mar 10, 2023 at 11:41:22PM +0100, Uwe Kleine-König wrote: > > > > > > > Hello, > > > > > > > > > > > > > > many bus remove functions return an integer which is a > > > > > > > historic misdesign that makes driver authors assume that > > > > > > > there is some kind of error handling in the upper layers. > > > > > > > This is wrong however and returning and error code only yields an > error message. > > > > > > > > > > > > > > This series improves the fsl-mc bus by changing the remove > > > > > > > callback to return no value instead. As a preparation all > > > > > > > drivers are changed to return zero before so that they don't > trigger the error message. > > > > > > > > > > > > Who is supposed to pick up this patch series (or point out a > > > > > > good reason for not taking it)? > > > > > > > > > > Previously Greg KH picked up MC bus patches. > > > > > > > > > > If no one is picking up them this time, I probably can take it > > > > > through the fsl soc tree. > > > > > > > > I guess Greg won't pick up this series as he didn't get a copy of > > > > it :-) > > > > > > > > Browsing through the history of drivers/bus/fsl-mc there is no > > > > consistent maintainer to see. So if you can take it, that's very > > > > appreciated. > > > > > > My mail was meant encouraging, maybe it was too subtile? I'll try again: > > > > > > Yes, please apply, that would be wonderful! > > > > Sorry for missing your previous email. I will do that. Thanks. > > Either you didn't apply this patch set yet, or your tree isn't in next. > Both variants would be great to be fixed. > > I have another change pending for drivers/bus/fsl-mc/fsl-mc-bus.c, would be > great to see these base patches in next first. I have applied them to the next branch of my tree. They will be part of the Linux-next soon. Regards, Leo
Re: [PATCH v2 RESEND] ASoC: fsl MPC52xx drivers require PPC_BESTCOMM
Hello maintainers,
I am still seeing these build errors on linux-next-20230530.
Is there a problem with the patch?
Thanks.
On 5/21/23 15:57, Randy Dunlap wrote:
> Both SND_MPC52xx_SOC_PCM030 and SND_MPC52xx_SOC_EFIKA select
> SND_SOC_MPC5200_AC97. The latter symbol depends on PPC_BESTCOMM,
> so the 2 former symbols should also depend on PPC_BESTCOMM since
> "select" does not follow any dependency chains.
>
> This prevents a kconfig warning and build errors:
>
> WARNING: unmet direct dependencies detected for SND_SOC_MPC5200_AC97
> Depends on [n]: SOUND [=y] && !UML && SND [=m] && SND_SOC [=m] &&
> SND_POWERPC_SOC [=m] && PPC_MPC52xx [=y] && PPC_BESTCOMM [=n]
> Selected by [m]:
> - SND_MPC52xx_SOC_PCM030 [=m] && SOUND [=y] && !UML && SND [=m] && SND_SOC
> [=m] && SND_POWERPC_SOC [=m] && PPC_MPC5200_SIMPLE [=y]
> - SND_MPC52xx_SOC_EFIKA [=m] && SOUND [=y] && !UML && SND [=m] && SND_SOC
> [=m] && SND_POWERPC_SOC [=m] && PPC_EFIKA [=y]
>
> ERROR: modpost: "mpc5200_audio_dma_destroy"
> [sound/soc/fsl/mpc5200_psc_ac97.ko] undefined!
> ERROR: modpost: "mpc5200_audio_dma_create"
> [sound/soc/fsl/mpc5200_psc_ac97.ko] undefined!
>
> Fixes: 40d9ec14e7e1 ("ASoC: remove BROKEN from Efika and pcm030 fabric
> drivers")
> Signed-off-by: Randy Dunlap
> Cc: Grant Likely
> Cc: Mark Brown
> Cc: Liam Girdwood
> Cc: Shengjiu Wang
> Cc: Xiubo Li
> Cc: alsa-de...@alsa-project.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> ---
> v2: use correct email address for Mark Brown.
>
> sound/soc/fsl/Kconfig |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff -- a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
> --- a/sound/soc/fsl/Kconfig
> +++ b/sound/soc/fsl/Kconfig
> @@ -243,7 +243,7 @@ config SND_SOC_MPC5200_AC97
>
> config SND_MPC52xx_SOC_PCM030
> tristate "SoC AC97 Audio support for Phytec pcm030 and WM9712"
> - depends on PPC_MPC5200_SIMPLE
> + depends on PPC_MPC5200_SIMPLE && PPC_BESTCOMM
> select SND_SOC_MPC5200_AC97
> select SND_SOC_WM9712
> help
> @@ -252,7 +252,7 @@ config SND_MPC52xx_SOC_PCM030
>
> config SND_MPC52xx_SOC_EFIKA
> tristate "SoC AC97 Audio support for bbplan Efika and STAC9766"
> - depends on PPC_EFIKA
> + depends on PPC_EFIKA && PPC_BESTCOMM
> select SND_SOC_MPC5200_AC97
> select SND_SOC_STAC9766
> help
--
~Randy
Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
On Mon, May 29, 2023 at 04:50:45PM +0200, Miguel Ojeda wrote: > Kees: what is the current stance on `[static N]` parameters? Something like: > > const char *kallsyms_lookup(unsigned long addr, > unsigned long *symbolsize, > unsigned long *offset, > - char **modname, char *namebuf); > + char **modname, char namebuf[static > KSYM_NAME_LEN]); > > makes the compiler complain about cases like these (even if trivial): > > arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small; > contains 128 elements, callee requires at least 512 > [-Werror,-Warray-bounds] > name = kallsyms_lookup(pc, , , NULL, tmpstr); > ^ ~~ > ./include/linux/kallsyms.h:86:29: note: callee declares array > parameter as static here > char **modname, char namebuf[static KSYM_NAME_LEN]); > ^ ~~ Wouldn't that be a good thing? (I.e. complain about the size mismatch?) > But I only see 2 files in the kernel using `[static N]` (from 2020 and > 2021). Should something else be used instead (e.g. `__counted_by`), > even if constexpr-sized?. Yeah, it seems pretty uncommon. I'd say traditionally arrays aren't based too often, rather structs containing them. But ultimately, yeah, everything could gain __counted_by and friends in the future. -- Kees Cook
Re: [PATCH v8 0/7] Add pci_dev_for_each_resource() helper and update users
On Fri, May 12, 2023 at 02:48:51PM -0500, Bjorn Helgaas wrote:
> On Fri, May 12, 2023 at 01:56:29PM +0300, Andy Shevchenko wrote:
> > On Tue, May 09, 2023 at 01:21:22PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote:
> > > > > Provide two new helper macros to iterate over PCI device resources and
> > > > > convert users.
> > >
> > > > Applied 2-7 to pci/resource for v6.4, thanks, I really like this!
> > >
> > > This is 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()")
> > > upstream now.
> > >
> > > Coverity complains about each use,
> >
> > It needs more clarification here. Use of reduced variant of the
> > macro or all of them? If the former one, then I can speculate that
> > Coverity (famous for false positives) simply doesn't understand `for
> > (type var; var ...)` code.
>
> True, Coverity finds false positives. It flagged every use in
> drivers/pci and drivers/pnp. It didn't mention the arch/alpha, arm,
> mips, powerpc, sh, or sparc uses, but I think it just didn't look at
> those.
>
> It flagged both:
>
> pbus_size_iopci_dev_for_each_resource(dev, r)
> pbus_size_mem pci_dev_for_each_resource(dev, r, i)
>
> Here's a spreadsheet with a few more details (unfortunately I don't
> know how to make it dump the actual line numbers or analysis like I
> pasted below, so "pci_dev_for_each_resource" doesn't appear). These
> are mostly in the "Drivers-PCI" component.
>
> https://docs.google.com/spreadsheets/d/1ohOJwxqXXoDUA0gwopgk-z-6ArLvhN7AZn4mIlDkHhQ/edit?usp=sharing
>
> These particular reports are in the "High Impact Outstanding" tab.
Where are we at? Are we going to ignore this because some Coverity
reports are false positives?
Bjorn
Re: [PATCH mm-unstable v2 04/10] kvm/arm64: make stage2 page tables RCU safe
On Tue, May 30, 2023 at 1:37 PM Oliver Upton wrote: > > Hi Yu, > > On Sat, May 27, 2023 at 02:13:07PM -0600, Yu Zhao wrote: > > On Sat, May 27, 2023 at 12:08 PM Oliver Upton > > wrote: > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > > index 3d61bd3e591d..bfbebdcb4ef0 100644 > > > --- a/arch/arm64/kvm/hyp/pgtable.c > > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > > @@ -1019,7 +1019,7 @@ static int stage2_unmap_walker(const struct > > > kvm_pgtable_visit_ctx *ctx, > > > > > > kvm_granule_size(ctx->level)); > > > > > > if (childp) > > > - mm_ops->put_page(childp); > > > + mm_ops->free_removed_table(childp, ctx->level); > > > > Thanks, Oliver. > > > > A couple of things I haven't had the chance to verify -- I'm hoping > > you could help clarify: > > 1. For unmapping, with free_removed_table(), wouldn't we have to look > > into the table we know it's empty unnecessarily? > > As it is currently implemented, yes. But, there's potential to fast-path > the implementation by checking page_count() before starting the walk. Do you mind posting another patch? I'd be happy to ack it, as well as the one you suggested above. > > 2. For remapping and unmapping, how does free_removed_table() put the > > final refcnt on the table passed in? (Previously we had > > put_page(childp) in stage2_map_walk_table_post(). So I'm assuming we'd > > have to do something equivalent with free_removed_table().) > > Heh, that's a bug, and an embarrassing one at that! > > Sent out a fix for that, since it would appear we leak memory on > table->block transitions. PTAL if you have a chance. > > https://lore.kernel.org/all/20230530193213.1663411-1-oliver.up...@linux.dev/ Awesome.
Re: [PATCH mm-unstable v2 04/10] kvm/arm64: make stage2 page tables RCU safe
Hi Yu, On Sat, May 27, 2023 at 02:13:07PM -0600, Yu Zhao wrote: > On Sat, May 27, 2023 at 12:08 PM Oliver Upton wrote: > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > index 3d61bd3e591d..bfbebdcb4ef0 100644 > > --- a/arch/arm64/kvm/hyp/pgtable.c > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > @@ -1019,7 +1019,7 @@ static int stage2_unmap_walker(const struct > > kvm_pgtable_visit_ctx *ctx, > > > > kvm_granule_size(ctx->level)); > > > > if (childp) > > - mm_ops->put_page(childp); > > + mm_ops->free_removed_table(childp, ctx->level); > > Thanks, Oliver. > > A couple of things I haven't had the chance to verify -- I'm hoping > you could help clarify: > 1. For unmapping, with free_removed_table(), wouldn't we have to look > into the table we know it's empty unnecessarily? As it is currently implemented, yes. But, there's potential to fast-path the implementation by checking page_count() before starting the walk. > 2. For remapping and unmapping, how does free_removed_table() put the > final refcnt on the table passed in? (Previously we had > put_page(childp) in stage2_map_walk_table_post(). So I'm assuming we'd > have to do something equivalent with free_removed_table().) Heh, that's a bug, and an embarrassing one at that! Sent out a fix for that, since it would appear we leak memory on table->block transitions. PTAL if you have a chance. https://lore.kernel.org/all/20230530193213.1663411-1-oliver.up...@linux.dev/ -- Thanks, Oliver
Re: [PATCH] soc: fsl: qe: Replace all non-returning strlcpy with strscpy
Duplicate of https://lore.kernel.org/all/20230523021425.2406309-1-azeemshaik...@gmail.com/. Sorry about that. On Tue, May 30, 2023 at 12:00 PM 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(). > No return values were used, so direct replacement is safe. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh > --- > drivers/soc/fsl/qe/qe.c |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c > index b3c226eb5292..58746e570d14 100644 > --- a/drivers/soc/fsl/qe/qe.c > +++ b/drivers/soc/fsl/qe/qe.c > @@ -524,7 +524,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware) > * saved microcode information and put in the new. > */ > memset(_firmware_info, 0, sizeof(qe_firmware_info)); > - strlcpy(qe_firmware_info.id, firmware->id, > sizeof(qe_firmware_info.id)); > + strscpy(qe_firmware_info.id, firmware->id, > sizeof(qe_firmware_info.id)); > qe_firmware_info.extended_modes = > be64_to_cpu(firmware->extended_modes); > memcpy(qe_firmware_info.vtraps, firmware->vtraps, > sizeof(firmware->vtraps)); > @@ -599,7 +599,7 @@ struct qe_firmware_info *qe_get_firmware_info(void) > /* Copy the data into qe_firmware_info*/ > sprop = of_get_property(fw, "id", NULL); > if (sprop) > - strlcpy(qe_firmware_info.id, sprop, > + strscpy(qe_firmware_info.id, sprop, > sizeof(qe_firmware_info.id)); > > of_property_read_u64(fw, "extended-modes", >
[PATCH] soc: fsl: qe: Replace all non-returning strlcpy with strscpy
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(). No return values were used, so direct replacement is safe. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh --- drivers/soc/fsl/qe/qe.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index b3c226eb5292..58746e570d14 100644 --- a/drivers/soc/fsl/qe/qe.c +++ b/drivers/soc/fsl/qe/qe.c @@ -524,7 +524,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware) * saved microcode information and put in the new. */ memset(_firmware_info, 0, sizeof(qe_firmware_info)); - strlcpy(qe_firmware_info.id, firmware->id, sizeof(qe_firmware_info.id)); + strscpy(qe_firmware_info.id, firmware->id, sizeof(qe_firmware_info.id)); qe_firmware_info.extended_modes = be64_to_cpu(firmware->extended_modes); memcpy(qe_firmware_info.vtraps, firmware->vtraps, sizeof(firmware->vtraps)); @@ -599,7 +599,7 @@ struct qe_firmware_info *qe_get_firmware_info(void) /* Copy the data into qe_firmware_info*/ sprop = of_get_property(fw, "id", NULL); if (sprop) - strlcpy(qe_firmware_info.id, sprop, + strscpy(qe_firmware_info.id, sprop, sizeof(qe_firmware_info.id)); of_property_read_u64(fw, "extended-modes",
Re: [PATCH 09/10] watchdog/hardlockup: Move SMP barriers from common code to buddy code
On Fri 2023-05-26 18:41:39, Douglas Anderson wrote: > It's been suggested that since the SMP barriers are only potentially > useful for the buddy hardlockup detector, not the perf hardlockup > detector, that the barriers belong in the buddy code. Let's move them > and add clearer comments about why they're needed. > > Suggested-by: Petr Mladek > Signed-off-by: Douglas Anderson Reviewed-by: Petr Mladek Best Regards, Petr
Re: [PATCH 08/10] watchdog/buddy: Simplify the dependency for HARDLOCKUP_DETECTOR_PREFER_BUDDY
On Fri 2023-05-26 18:41:38, Douglas Anderson wrote: > The dependency for HARDLOCKUP_DETECTOR_PREFER_BUDDY was more > complicated than it needed to be. If the "perf" detector is available > and we have SMP then we have a choice, so enable the config based on > just those two config items. > > Suggested-by: Petr Mladek > Signed-off-by: Douglas Anderson Reviewed-by: Petr Mladek Best Regards, Petr
Re: [PATCH 07/10] watchdog/buddy: Don't copy the cpumask in watchdog_next_cpu()
On Fri 2023-05-26 18:41:37, Douglas Anderson wrote: > There's no reason to make a copy of the "watchdog_cpus" locally in > watchdog_next_cpu(). Making a copy wouldn't make things any more race > free and we're just reading the value so there's no need for a copy. > > Suggested-by: Petr Mladek > Signed-off-by: Douglas Anderson Reviewed-by: Petr Mladek Best Regards, Petr
Re: [PATCH 06/10] watchdog/buddy: Cleanup how watchdog_buddy_check_hardlockup() is called
On Fri 2023-05-26 18:41:36, Douglas Anderson wrote:
> In the patch ("watchdog/hardlockup: detect hard lockups using
> secondary (buddy) CPUs"), we added a call from the common watchdog.c
> file into the buddy. That call could be done more cleanly.
> Specifically:
> 1. If we move the call into watchdog_hardlockup_kick() then it keeps
>watchdog_timer_fn() simpler.
> 2. We don't need to pass an "unsigned long" to the buddy for the timer
>count. In the patch ("watchdog/hardlockup: add a "cpu" param to
>watchdog_hardlockup_check()") the count was changed to "atomic_t"
>which is backed by an int, so we should match types.
>
> Suggested-by: Petr Mladek
> Signed-off-by: Douglas Anderson
The change looks fine:
Reviewed-by: Petr Mladek
That said, I would prefer to squash it into the patch ("watchdog/hardlockup:
detect hard lockups using secondary (buddy) CPUs"). It would remove
some back and forth churn in the git history. But it is up to Andrew.
Best Regards,
Petr
Re: [PATCH 05/10] watchdog/hardlockup: remove softlockup comment in touch_nmi_watchdog()
On Fri 2023-05-26 18:41:35, Douglas Anderson wrote:
> In the patch ("watchdog/hardlockup: add comments to
> touch_nmi_watchdog()") we adjusted some comments for
> touch_nmi_watchdog(). The comment about the softlockup had a typo and
> were also felt to be too obvious. Remove it.
>
> Suggested-by: Petr Mladek
> Signed-off-by: Douglas Anderson
Reviewed-by: Petr Mladek
Best Regards,
Petr
Re: [PATCH 04/10] watchdog/hardlockup: In watchdog_hardlockup_check() use cpumask_copy()
On Fri 2023-05-26 18:41:34, Douglas Anderson wrote:
> In the patch ("watchdog/hardlockup: add a "cpu" param to
> watchdog_hardlockup_check()") we started using a cpumask to keep track
> of which CPUs to backtrace. When setting up this cpumask, it's better
> to use cpumask_copy() than to just copy the structure directly. Fix this.
>
> Suggested-by: Petr Mladek
> Signed-off-by: Douglas Anderson
Reviewed-by: Petr Mladek
Best Regards,
Petr
Re: [PATCH 03/10] watchdog/hardlockup: Don't use raw_cpu_ptr() in watchdog_hardlockup_kick()
On Fri 2023-05-26 18:41:33, Douglas Anderson wrote:
> In the patch ("watchdog/hardlockup: add a "cpu" param to
> watchdog_hardlockup_check()") there was no reason to use
> raw_cpu_ptr(). Using this_cpu_ptr() works fine.
>
> Suggested-by: Petr Mladek
> Signed-off-by: Douglas Anderson
Reviewed-by: Petr Mladek
Best Regards,
Petr
Re: [PATCH 02/10] watchdog/hardlockup: HAVE_NMI_WATCHDOG must implement watchdog_hardlockup_probe()
On Fri 2023-05-26 18:41:32, Douglas Anderson wrote:
> Right now there is one arch (sparc64) that selects HAVE_NMI_WATCHDOG
> without selecting HAVE_HARDLOCKUP_DETECTOR_ARCH. Because of that one
> architecture, we have some special case code in the watchdog core to
> handle the fact that watchdog_hardlockup_probe() isn't implemented.
>
> Let's implement watchdog_hardlockup_probe() for sparc64 and get rid of
> the special case.
>
> As a side effect of doing this, code inspection tells us that we could
> fix a minor bug where the system won't properly realize that NMI
> watchdogs are disabled. Specifically, on powerpc if
> CONFIG_PPC_WATCHDOG is turned off the arch might still select
> CONFIG_HAVE_HARDLOCKUP_DETECTOR_ARCH which selects
> CONFIG_HAVE_NMI_WATCHDOG. Since CONFIG_PPC_WATCHDOG was off then
> nothing will override the "weak" watchdog_hardlockup_probe() and we'll
> fallback to looking at CONFIG_HAVE_NMI_WATCHDOG.
>
> Suggested-by: Petr Mladek
> Signed-off-by: Douglas Anderson
Looks good:
Reviewed-by: Petr Mladek
> ---
> Though this does fix a minor bug, I didn't mark this as "Fixes"
> because it's super minor. One could also argue that this wasn't a bug
> at all but simply was never an implemented feature. The code that
> added some amount of dynamicness here was commit a994a3147e4c
> ("watchdog/hardlockup/perf: Implement init time detection of perf")
> which, as per the title, was only intending to make "perf"
> dynamic. The old NMI watchdog presumably has never been handled
> dynamically.
I agree that it is a minor bug and Fixes tag is not needed.
Another motivation is that all the dependencies are quite
complicated. IMHO, it is not worth spending time on
reviewing potential backports.
That said, I am afraid that the artificial intelligence will nominate
this patch for stable backports even without the Fixes tag.
You know, there are the words "fix" and "bug" in the commit message ;-)
Best Regards,
Petr
Re: [PATCH 01/10] watchdog/hardlockup: Keep kernel.nmi_watchdog sysctl as 0444 if probe fails
On Fri 2023-05-26 18:41:31, Douglas Anderson wrote:
> The permissions for the kernel.nmi_watchdog sysctl have always been
> set at compile time despite the fact that a watchdog can fail to
> probe. Let's fix this and set the permissions based on whether the
> hardlockup detector actually probed.
>
> Fixes: a994a3147e4c ("watchdog/hardlockup/perf: Implement init time detection
> of perf")
> Reported-by: Petr Mladek
> Closes: https://lore.kernel.org/r/ZHCn4hNxFpY5-9Ki@alley
> Signed-off-by: Douglas Anderson
Looks good to me:
Reviewed-by: Petr Mladek
Best Regards,
Petr
Re: [PATCH 0/6] bus: fsl-mc: Make remove function return void
Hello, On Mon, May 08, 2023 at 04:57:00PM -0500, Li Yang wrote: > On Mon, May 8, 2023 at 8:44 AM Uwe Kleine-König > wrote: > > On Thu, Apr 13, 2023 at 08:00:04AM +0200, Uwe Kleine-König wrote: > > > On Wed, Apr 12, 2023 at 09:30:05PM +, Leo Li wrote: > > > > > On Fri, Mar 10, 2023 at 11:41:22PM +0100, Uwe Kleine-König wrote: > > > > > > Hello, > > > > > > > > > > > > many bus remove functions return an integer which is a historic > > > > > > misdesign that makes driver authors assume that there is some kind > > > > > > of > > > > > > error handling in the upper layers. This is wrong however and > > > > > > returning and error code only yields an error message. > > > > > > > > > > > > This series improves the fsl-mc bus by changing the remove callback > > > > > > to > > > > > > return no value instead. As a preparation all drivers are changed to > > > > > > return zero before so that they don't trigger the error message. > > > > > > > > > > Who is supposed to pick up this patch series (or point out a good > > > > > reason for > > > > > not taking it)? > > > > > > > > Previously Greg KH picked up MC bus patches. > > > > > > > > If no one is picking up them this time, I probably can take it through > > > > the fsl soc tree. > > > > > > I guess Greg won't pick up this series as he didn't get a copy of it :-) > > > > > > Browsing through the history of drivers/bus/fsl-mc there is no > > > consistent maintainer to see. So if you can take it, that's very > > > appreciated. > > > > My mail was meant encouraging, maybe it was too subtile? I'll try again: > > > > Yes, please apply, that would be wonderful! > > Sorry for missing your previous email. I will do that. Thanks. Either you didn't apply this patch set yet, or your tree isn't in next. Both variants would be great to be fixed. I have another change pending for drivers/bus/fsl-mc/fsl-mc-bus.c, would be great to see these base patches in next first. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v2 0/3] Remove iommu_group_remove_device() from fsl
On Tue, May 30, 2023 at 10:03:53PM +1000, Michael Ellerman wrote: > Jason Gunthorpe writes: > > On Tue, May 23, 2023 at 08:26:32AM +0200, Joerg Roedel wrote: > >> On Tue, May 16, 2023 at 09:35:25PM -0300, Jason Gunthorpe wrote: > >> > With POWER SPAPR now having a real iommu driver and using the normal > >> > group > >> > lifecycle stuff fixing FSL will leave only VFIO's no-iommu support as a > >> > user for the iommu_group_add/remove_device() calls. This will help > >> > simplify the understanding of what the core code should be doing for > >> > these > >> > functions. > >> > > >> > Fix FSL to not need to call iommu_group_remove_device() at all. > >> > > >> > v2: > >> > - Change the approach to use driver_managed_dma > >> > - Really simplify fsl_pamu_device_group() and just put everything in one > >> >function > >> > - New patch to make missing OF properties a probe failure > >> > v1: > >> > https://lore.kernel.org/r/0-v1-1421774b874b+167-ppc_device_group_...@nvidia.com > >> > > >> > Jason Gunthorpe (3): > >> > iommu/fsl: Always allocate a group for non-pci devices > >> > iommu/fsl: Move ENODEV to fsl_pamu_probe_device() > >> > iommu/fsl: Use driver_managed_dma to allow VFIO to work > >> > > >> > arch/powerpc/sysdev/fsl_pci.c | 1 + > >> > drivers/iommu/fsl_pamu_domain.c | 123 +--- > >> > 2 files changed, 36 insertions(+), 88 deletions(-) > >> > >> Any chance someone can test this on real hardware? > > > > There isn't even a MAINTAINERS entry for this, and the git log looks > > pretty dead for a long time. I tried to cc people who might care, > > but I'm not so optimistic - unless Li says something. > > I guess it falls under LINUX FOR POWERPC EMBEDDED PPC83XX AND PPC85XX, > but that's basically orphaned these days. Basically all the FSL/NXP > powerpc code is orphaned, although there are still some users. :\ > But looks like this driver is powerpc only. Yes > I do see some changes in dmesg, eg: > > -fsl-pci ffe27.pcie: Removing from iommu group 61 > -pci 0003:00:00.0: Adding to iommu group 60 > +pci 0003:00:00.0: Adding to iommu group 64 > > And lots more like that. Yes, we expected that the groups would renumber. > Anything else I can check easily? Wow Great, I think that is a Tested-by. :) Honestly booting at all is 99% of the battle.. This system looks like it has "partitionable end points" so I expect it to all work, all this does is create a group for the controller itself, which you saw in the boot with this diff: -fsl-pci ffe27.pcie: Removing from iommu group 61 Which is harmless as long as its group is singleton. So, I don't think there is more you can do with this system. Joerg, this seems like enough, lets go ahead please :) Thanks, Jason
RE: [PATCH 1/2] hexagon/traps.c: use KSYM_NAME_LEN in array size
From: Maninder Singh
> Sent: 29 May 2023 12:14
>
> kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
> writes on index "KSYM_NAME_LEN - 1".
>
> Thus array size should be KSYM_NAME_LEN.
>
> for hexagon it was defined as "128" directly.
> and commit '61968dbc2d5d' changed define value to 512,
> So both were missed to update with new size.
The only safe way to pass a fixed size string is to embed the char[] in
a structure and pass the structure address.
Pretty much anything else is doomed to be buggy.
Whether is it actually sane to require the caller allocate
such a large buffer (hi rust) is another matter entirely.
David
>
> Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")
>
> Co-developed-by: Onkarnath
> Signed-off-by: Onkarnath
> Signed-off-by: Maninder Singh
> ---
> arch/hexagon/kernel/traps.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/hexagon/kernel/traps.c b/arch/hexagon/kernel/traps.c
> index 6447763ce5a9..65b30b6ea226 100644
> --- a/arch/hexagon/kernel/traps.c
> +++ b/arch/hexagon/kernel/traps.c
> @@ -82,7 +82,7 @@ static void do_show_stack(struct task_struct *task,
> unsigned long *fp,
> const char *name = NULL;
> unsigned long *newfp;
> unsigned long low, high;
> - char tmpstr[128];
> + char tmpstr[KSYM_NAME_LEN];
> char *modname;
> int i;
>
> --
> 2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH 1/2] hexagon/traps.c: use KSYM_NAME_LEN in array size
On Mon, May 29, 2023 at 1:14 PM Maninder Singh wrote:
>
> kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
> writes on index "KSYM_NAME_LEN - 1".
>
> Thus array size should be KSYM_NAME_LEN.
>
> for hexagon it was defined as "128" directly.
> and commit '61968dbc2d5d' changed define value to 512,
> So both were missed to update with new size.
>
> Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")
>
> Co-developed-by: Onkarnath
> Signed-off-by: Onkarnath
> Signed-off-by: Maninder Singh
With the updated commit hash:
Reviewed-by: Miguel Ojeda
Cheers,
Miguel
Re: [PATCH 2/2] powerpc/xmon: use KSYM_NAME_LEN in array size
On Mon, May 29, 2023 at 1:14 PM Maninder Singh wrote: > > +static char tmpstr[KSYM_NAME_LEN]; Reviewed-by: Miguel Ojeda Side-note: in `get_function_bounds()`, I see `kallsyms_lookup()` being used, but the name seems discarded? Can `kallsyms_lookup_size_offset()` be used instead, thus avoiding the usage of the buffer there to begin with? Side-note 2: in `scanhex()`, I see a loop `i<63` using `tmpstr` which then is used to do a `kallsyms_lookup_name()`, so I guess symbols larger than 64 couldn't be found. I have no idea about what are the external constraints here, but perhaps it is possible to increase the `line` buffer etc. to then allow for bigger symbols to be found. Cheers, Miguel
Re: [PATCH v2 0/3] Remove iommu_group_remove_device() from fsl
Jason Gunthorpe writes: > On Tue, May 23, 2023 at 08:26:32AM +0200, Joerg Roedel wrote: >> On Tue, May 16, 2023 at 09:35:25PM -0300, Jason Gunthorpe wrote: >> > With POWER SPAPR now having a real iommu driver and using the normal group >> > lifecycle stuff fixing FSL will leave only VFIO's no-iommu support as a >> > user for the iommu_group_add/remove_device() calls. This will help >> > simplify the understanding of what the core code should be doing for these >> > functions. >> > >> > Fix FSL to not need to call iommu_group_remove_device() at all. >> > >> > v2: >> > - Change the approach to use driver_managed_dma >> > - Really simplify fsl_pamu_device_group() and just put everything in one >> >function >> > - New patch to make missing OF properties a probe failure >> > v1: >> > https://lore.kernel.org/r/0-v1-1421774b874b+167-ppc_device_group_...@nvidia.com >> > >> > Jason Gunthorpe (3): >> > iommu/fsl: Always allocate a group for non-pci devices >> > iommu/fsl: Move ENODEV to fsl_pamu_probe_device() >> > iommu/fsl: Use driver_managed_dma to allow VFIO to work >> > >> > arch/powerpc/sysdev/fsl_pci.c | 1 + >> > drivers/iommu/fsl_pamu_domain.c | 123 +--- >> > 2 files changed, 36 insertions(+), 88 deletions(-) >> >> Any chance someone can test this on real hardware? > > There isn't even a MAINTAINERS entry for this, and the git log looks > pretty dead for a long time. I tried to cc people who might care, > but I'm not so optimistic - unless Li says something. I guess it falls under LINUX FOR POWERPC EMBEDDED PPC83XX AND PPC85XX, but that's basically orphaned these days. Basically all the FSL/NXP powerpc code is orphaned, although there are still some users. And things are somewhat complicated because some of the drivers are also used on their ARM SOCs, so those still get maintained from the ARM side. But looks like this driver is powerpc only. Turns out I do have a machine that will probe this driver. AFAICS this series doesn't regress it, but that's just booting. I don't have it setup to test KVM/VFIO etc. I do see some changes in dmesg, eg: -fsl-pci ffe27.pcie: Removing from iommu group 61 -pci 0003:00:00.0: Adding to iommu group 60 +pci 0003:00:00.0: Adding to iommu group 64 And lots more like that. Anything else I can check easily? cheers
Re: [PATCH] powerpc: fix debugfs_create_dir error checking
Greg KH writes:
> On Sun, May 28, 2023 at 01:16:44PM +0530, mirim...@outlook.com wrote:
>> From: Immad Mir
>>
>> The debugfs_create_dir returns ERR_PTR incase of an error and the
>> correct way of checking it by using the IS_ERR inline function, and
>> not the simple null comparision. This patch fixes this.
>>
>> Suggested-By: Ivan Orlov
>> Signed-off-by: Immad Mir
>> ---
>> arch/powerpc/platforms/powernv/opal-xscom.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal-xscom.c
>> b/arch/powerpc/platforms/powernv/opal-xscom.c
>> index 6b4eed2ef..262cd6fac 100644
>> --- a/arch/powerpc/platforms/powernv/opal-xscom.c
>> +++ b/arch/powerpc/platforms/powernv/opal-xscom.c
>> @@ -168,7 +168,7 @@ static int scom_debug_init_one(struct dentry *root,
>> struct device_node *dn,
>> ent->path.size = strlen((char *)ent->path.data);
>>
>> dir = debugfs_create_dir(ent->name, root);
>> -if (!dir) {
>> +if (IS_ERR(dir)) {
>> kfree(ent->path.data);
>> kfree(ent);
>> return -1;
>
> Why is this driver caring if debugfs is working or not at all? It
> should just ignore the error and keep moving forward.
It's creating directories and then creating files in those directories.
So I think it makes sense that it checks that the directory was created
successfully. It doesn't check whether the files were created.
> And -1 is not a valid error number :(
It's EPERM :) - but yeah probably not really the right error in this
case.
Still I think this patch is an improvement so I'll plan to merge it.
cheers
[PATCH] ASoC: fsl_sai: Enable BCI bit if SAI works on synchronous mode with BYP asserted
There's an issue on SAI synchronous mode that TX/RX side can't get BCLK
from RX/TX it sync with if BYP bit is asserted. It's a workaround to
fix it that enable SION of IOMUX pad control and assert BCI.
For example if TX sync with RX which means both TX and RX are using clk
form RX and BYP=1. TX can get BCLK only if the following two conditions
are valid:
1. SION of RX BCLK IOMUX pad is set to 1
2. BCI of TX is set to 1
Signed-off-by: Chancel Liu
---
sound/soc/fsl/fsl_sai.c | 11 +--
sound/soc/fsl/fsl_sai.h | 1 +
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index d9344025dc16..5e09f634c61b 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -491,14 +491,21 @@ static int fsl_sai_set_bclk(struct snd_soc_dai *dai, bool
tx, u32 freq)
regmap_update_bits(sai->regmap, reg, FSL_SAI_CR2_MSEL_MASK,
FSL_SAI_CR2_MSEL(sai->mclk_id[tx]));
- if (savediv == 1)
+ if (savediv == 1) {
regmap_update_bits(sai->regmap, reg,
FSL_SAI_CR2_DIV_MASK | FSL_SAI_CR2_BYP,
FSL_SAI_CR2_BYP);
- else
+ if (fsl_sai_dir_is_synced(sai, adir))
+ regmap_update_bits(sai->regmap, FSL_SAI_xCR2(tx, ofs),
+ FSL_SAI_CR2_BCI, FSL_SAI_CR2_BCI);
+ else
+ regmap_update_bits(sai->regmap, FSL_SAI_xCR2(tx, ofs),
+ FSL_SAI_CR2_BCI, 0);
+ } else {
regmap_update_bits(sai->regmap, reg,
FSL_SAI_CR2_DIV_MASK | FSL_SAI_CR2_BYP,
savediv / 2 - 1);
+ }
if (sai->soc_data->max_register >= FSL_SAI_MCTL) {
/* SAI is in master mode at this point, so enable MCLK */
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 3eb994aef36a..8254c3547b87 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -116,6 +116,7 @@
/* SAI Transmit and Receive Configuration 2 Register */
#define FSL_SAI_CR2_SYNC BIT(30)
+#define FSL_SAI_CR2_BCIBIT(28)
#define FSL_SAI_CR2_MSEL_MASK (0x3 << 26)
#define FSL_SAI_CR2_MSEL_BUS 0
#define FSL_SAI_CR2_MSEL_MCLK1 BIT(26)
--
2.25.1
[PATCH] powerpc/tools: Pass -mabi=elfv2 to gcc-check-mprofile-kernel.sh
Toolchains don't always default to the ELFv2 ABI. This is true with at
least the kernel.org toolchains. As such, pass -mabi=elfv2 explicitly to
ensure that we are testing against the correct compiler output.
Signed-off-by: Naveen N Rao
---
The script works fine without this change, so this is not a fix per se.
I felt it is better to be explicit about the ABI here to be sure about
what we are testing against.
This applies atop Nick's patch to enable -mprofile-kernel for ELFv2 BE:
http://lore.kernel.org/20230506011814.8766-1-npig...@gmail.com
- Naveen
arch/powerpc/tools/gcc-check-mprofile-kernel.sh | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/tools/gcc-check-mprofile-kernel.sh
b/arch/powerpc/tools/gcc-check-mprofile-kernel.sh
index e78c599251ff35..abd6b9b8f07e3e 100755
--- a/arch/powerpc/tools/gcc-check-mprofile-kernel.sh
+++ b/arch/powerpc/tools/gcc-check-mprofile-kernel.sh
@@ -7,20 +7,20 @@ set -o pipefail
# To debug, uncomment the following line
# set -x
-# -mprofile-kernel is only supported on 64le, so this should not be invoked
-# for other targets. Therefore we can pass in -m64 and -mlittle-endian
-# explicitly, to take care of toolchains defaulting to other targets.
+# -mprofile-kernel is only supported on ppc64, so this should not be invoked
+# for other targets. Therefore we can pass in -m64 and -mabi explicitly, to
+# take care of toolchains defaulting to other targets.
# Test whether the compile option -mprofile-kernel exists and generates
# profiling code (ie. a call to _mcount()).
echo "int func() { return 0; }" | \
-$* -m64 -S -x c -O2 -p -mprofile-kernel - -o - \
+$* -m64 -mabi=elfv2 -S -x c -O2 -p -mprofile-kernel - -o - \
2> /dev/null | grep -q "_mcount"
# Test whether the notrace attribute correctly suppresses calls to _mcount().
echo -e "#include \nnotrace int func() { return 0; }" | \
-$* -m64 -S -x c -O2 -p -mprofile-kernel - -o - \
+$* -m64 -mabi=elfv2 -S -x c -O2 -p -mprofile-kernel - -o - \
2> /dev/null | grep -q "_mcount" && \
exit 1
base-commit: 7b2f56d76feff3b494d6e77950ab97987323d3c5
prerequisite-patch-id: 502430fc318f92c1fe5f3b482ca8d798232b0516
prerequisite-patch-id: 3862cf6dc646260228e50b70c316cf15b1d7f384
prerequisite-patch-id: 859f60071f4e425c806fc7fe6c59e268232050a4
prerequisite-patch-id: ef23f712e50f106d689a550dae0f816285c1db3b
prerequisite-patch-id: 8c6d31bb6ac4e4bef086fe502efa660ae99a96ca
--
2.40.1
Re: [PATCH 1/2] hexagon/traps.c: use KSYM_NAME_LEN in array size
On Mon 2023-05-29 16:43:36, Maninder Singh wrote:
> kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
> writes on index "KSYM_NAME_LEN - 1".
>
> Thus array size should be KSYM_NAME_LEN.
>
> for hexagon it was defined as "128" directly.
> and commit '61968dbc2d5d' changed define value to 512,
> So both were missed to update with new size.
>
> Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")
As mentioned by Michael Ellerman for the 2nd patch, the right upstream
commit is:
b8a94bfb3395 ("kallsyms: increase maximum kernel symbol length to 512")
> Co-developed-by: Onkarnath
> Signed-off-by: Onkarnath
> Signed-off-by: Maninder Singh
With the updated commit hash:
Reviewed-by: Petr Mladek
Best Regards,
Petr
RE: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
Hi Peter, > > The best solution would be to pass the buffer size as an extra > parameter. Especially when some code passes buffers that are > allocated/reserved dynamically. > > Sigh, I am not sure how many changes it would require in kallsyms > API and all the callers. But it would be really appreciated, IMHO. > yes we already prepared size changes 5-6 months back: https://lore.kernel.org/lkml/yontol4zc4cyt...@infradead.org/t/ [PATCH 1/5] kallsyms: pass buffer size in sprint_* APIs But at that time new API development(for replacement of seq_buf) was in progress and we decided to wait for that completion. https://lore.kernel.org/r/20220604193042.1674951-2-kent.overstr...@gmail.com https://lore.kernel.org/r/20220604193042.1674951-4-kent.overstr...@gmail.com As I checeked these APIs are not pushed to mainline. we will try to prepare new patch set for kallsym changes again with seq_buf to take care of length argument. Thanks, Maninder Singh
Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
On Mon 2023-05-29 16:50:45, Miguel Ojeda wrote: > On Mon, May 29, 2023 at 1:08 PM Maninder Singh > wrote: > > > > I Will add co-developed-by` tag. > > because this change was identified while we were working on kallsyms some > > time back. > > https://lore.kernel.org/lkml/yontol4zc4cyt...@infradead.org/t/ > > > > this patch set is pending and we will start working on that again, so i > > thought better > > to send bugfix first. > > Sounds good to me! > > (Fixed Wedson's email address) > > > Yes, I think second buffer was not related to kallsyms, so I have not > > touched that. > > Kees: what is the current stance on `[static N]` parameters? Something like: > > const char *kallsyms_lookup(unsigned long addr, > unsigned long *symbolsize, > unsigned long *offset, > - char **modname, char *namebuf); > + char **modname, char namebuf[static > KSYM_NAME_LEN]); > > makes the compiler complain about cases like these (even if trivial): > > arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small; > contains 128 elements, callee requires at least 512 > [-Werror,-Warray-bounds] > name = kallsyms_lookup(pc, , , NULL, tmpstr); > ^ ~~ > ./include/linux/kallsyms.h:86:29: note: callee declares array > parameter as static here > char **modname, char namebuf[static KSYM_NAME_LEN]); > ^ ~~ > > But I only see 2 files in the kernel using `[static N]` (from 2020 and > 2021). Should something else be used instead (e.g. `__counted_by`), > even if constexpr-sized?. > > Also, I went through the other callers to `kallsyms_lookup` to see > other issues -- one I am not sure about is `fetch_store_symstring` in > `kernel/trace/trace_probe_tmpl.h`. Steven/Masami: is that "with max > length" in the function docs enough? Is it 0x? The best solution would be to pass the buffer size as an extra parameter. Especially when some code passes buffers that are allocated/reserved dynamically. Sigh, I am not sure how many changes it would require in kallsyms API and all the callers. But it would be really appreciated, IMHO. Best Regards, Petr
Re: [PATCH] powerpc/kcsan: Properly instrument arch_spin_unlock()
On Mon, 29 May 2023 at 17:50, Christophe Leroy
wrote:
>
> The following boottime error is encountered with SMP kernel:
>
> kcsan: improperly instrumented type=(0): arch_spin_unlock(_spinlock)
> kcsan: improperly instrumented type=(0): spin_unlock(_spinlock)
> kcsan: improperly instrumented type=(KCSAN_ACCESS_WRITE):
> arch_spin_unlock(_spinlock)
> kcsan: improperly instrumented type=(KCSAN_ACCESS_WRITE):
> spin_unlock(_spinlock)
> kcsan: improperly instrumented type=(KCSAN_ACCESS_WRITE |
> KCSAN_ACCESS_COMPOUND): arch_spin_unlock(_spinlock)
> kcsan: improperly instrumented type=(KCSAN_ACCESS_WRITE |
> KCSAN_ACCESS_COMPOUND): spin_unlock(_spinlock)
> kcsan: selftest: test_barrier failed
> kcsan: selftest: 2/3 tests passed
> Kernel panic - not syncing: selftests failed
>
> Properly instrument arch_spin_unlock() with kcsan_mb().
>
> Signed-off-by: Christophe Leroy
Acked-by: Marco Elver
> ---
> arch/powerpc/include/asm/simple_spinlock.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/simple_spinlock.h
> b/arch/powerpc/include/asm/simple_spinlock.h
> index 9dcc7e9993b9..4dd12dcb9ef8 100644
> --- a/arch/powerpc/include/asm/simple_spinlock.h
> +++ b/arch/powerpc/include/asm/simple_spinlock.h
> @@ -15,6 +15,7 @@
> * (the type definitions are in asm/simple_spinlock_types.h)
> */
> #include
> +#include
> #include
> #include
> #include
> @@ -126,6 +127,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
>
> static inline void arch_spin_unlock(arch_spinlock_t *lock)
> {
> + kcsan_mb();
> __asm__ __volatile__("# arch_spin_unlock\n\t"
> PPC_RELEASE_BARRIER: : :"memory");
> lock->slock = 0;
> --
> 2.40.1
>
Re: [PATCH 2/2] powerpc/xmon: use KSYM_NAME_LEN in array size
Maninder Singh writes:
> kallsyms_lookup which in turn calls for kallsyms_lookup_buildid()
> writes on index "KSYM_NAME_LEN - 1".
>
> Thus array size should be KSYM_NAME_LEN.
>
> for powerpc it was defined as "128" directly.
> and commit '61968dbc2d5d' changed define value to 512,
> So both were missed to update with new size.
>
> Fixes: 61968dbc2d5d ("kallsyms: increase maximum kernel symbol length to 512")
AFAICS that's the wrong sha.
That commit appears in linux-next, but the commit that actually went
into mainline is:
b8a94bfb3395 ("kallsyms: increase maximum kernel symbol length to 512")
So I'll update the change log to refer to that.
cheers
> Co-developed-by: Onkarnath
> Signed-off-by: Onkarnath
> Signed-off-by: Maninder Singh
> ---
> arch/powerpc/xmon/xmon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 728d3c257e4a..70c4c59a1a8f 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -88,7 +88,7 @@ static unsigned long ndump = 64;
> static unsigned long nidump = 16;
> static unsigned long ncsum = 4096;
> static int termch;
> -static char tmpstr[128];
> +static char tmpstr[KSYM_NAME_LEN];
> static int tracing_enabled;
>
> static long bus_error_jmp[JMP_BUF_LEN];
> --
> 2.17.1
Re: [PATCH] powerpc: Fail build if using recordmcount with binutils v2.37
Michael Ellerman wrote:
Naveen N Rao writes:
binutils v2.37 drops unused section symbols, which prevents recordmcount
from capturing mcount locations in sections that have no non-weak
symbols. This results in a build failure with a message such as:
Cannot find symbol for section 12: .text.perf_callchain_kernel.
kernel/events/callchain.o: failed
The change to binutils was reverted for v2.38, so this behavior is
specific to binutils v2.37:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=c09c8b42021180eee9495bd50d8b35e683d3901b
Objtool is able to cope with such sections, so this issue is specific to
recordmcount.
Fail the build and print a warning if binutils v2.37 is detected and if
we are using recordmcount.
Cc: sta...@vger.kernel.org
Suggested-by: Joel Stanley
Signed-off-by: Naveen N Rao
---
arch/powerpc/Makefile | 8
1 file changed, 8 insertions(+)
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index dca73f673d7046..f0540c1f1377c8 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -409,3 +409,11 @@ checkbin:
echo -n '*** Please use a different binutils version.' ; \
false ; \
fi
+ @if test "x${CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT}" = "xy" -a \
+ "x${CONFIG_LD_IS_BFD}" = "xy" -a \
+ "x$(call ld-ifversion, -eq, 23700, y)" = "xy" ; then \
+ echo -n '*** binutils 2.37 drops unused section symbols, which
recordmcount ' ; \
+ echo 'is unable to handle.' ; \
+ echo '*** Please use a different binutils version.' ; \
+ false ; \
+ fi
Thanks for doing this.
Masahiro wanted to remove ld-ifversion, he suggested to just check
CONFIG_LD_VERSION directly instead. Mind doing a v2 with that change?
https://lore.kernel.org/all/CAK7LNAQWtDHOs=k+qznt5u1widv86tchkj4zoer4wtvrb97...@mail.gmail.com/
I have posted a v2 with just that change. Let me know if you are
planning to pick up Masahiro's patch above and if you want me to base my
changes atop that (just have to add back archprepare/checkbin).
- Naveen
[PATCH v2] powerpc: Fail build if using recordmcount with binutils v2.37
binutils v2.37 drops unused section symbols, which prevents recordmcount
from capturing mcount locations in sections that have no non-weak
symbols. This results in a build failure with a message such as:
Cannot find symbol for section 12: .text.perf_callchain_kernel.
kernel/events/callchain.o: failed
The change to binutils was reverted for v2.38, so this behavior is
specific to binutils v2.37:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=c09c8b42021180eee9495bd50d8b35e683d3901b
Objtool is able to cope with such sections, so this issue is specific to
recordmcount.
Fail the build and print a warning if binutils v2.37 is detected and if
we are using recordmcount.
Cc: sta...@vger.kernel.org
Suggested-by: Joel Stanley
Signed-off-by: Naveen N Rao
---
v2: Check directly against $CONFIG_LD_VERSION
arch/powerpc/Makefile | 8
1 file changed, 8 insertions(+)
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index dca73f673d7046..fc76eb9830fdfa 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -409,3 +409,11 @@ checkbin:
echo -n '*** Please use a different binutils version.' ; \
false ; \
fi
+ @if test "x${CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT}" = "xy" -a \
+ "x${CONFIG_LD_IS_BFD}" = "xy" -a \
+ "${CONFIG_LD_VERSION}" = "23700" ; then \
+ echo -n '*** binutils 2.37 drops unused section symbols, which
recordmcount ' ; \
+ echo 'is unable to handle.' ; \
+ echo '*** Please use a different binutils version.' ; \
+ false ; \
+ fi
base-commit: 7b2f56d76feff3b494d6e77950ab97987323d3c5
--
2.40.1
