Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Jani,
On Thu, Jul 13, 2023 at 11:03 AM Jani Nikula wrote:
> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> > On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> >> On Wed, 12 Jul 2023, Uwe Kleine-König
> >> wrote:
> >> > while I debugged an issue in the imx-lcdc driver I was constantly
> >> > irritated about struct drm_device pointer variables being named "dev"
> >> > because with that name I usually expect a struct device pointer.
> >> >
> >> > I think there is a big benefit when these are all renamed to "drm_dev".
> >> > I have no strong preference here though, so "drmdev" or "drm" are fine
> >> > for me, too. Let the bikesheding begin!
> >> >
> >> > Some statistics:
> >> >
> >> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort |
> >> > uniq -c | sort -n
> >> > 1 struct drm_device *adev_to_drm
> >> > 1 struct drm_device *drm_
> >> > 1 struct drm_device *drm_dev
> >> > 1 struct drm_device*drm_dev
> >> > 1 struct drm_device *pdev
> >> > 1 struct drm_device *rdev
> >> > 1 struct drm_device *vdev
> >> > 2 struct drm_device *dcss_drv_dev_to_drm
> >> > 2 struct drm_device **ddev
> >> > 2 struct drm_device *drm_dev_alloc
> >> > 2 struct drm_device *mock
> >> > 2 struct drm_device *p_ddev
> >> > 5 struct drm_device *device
> >> > 9 struct drm_device * dev
> >> > 25 struct drm_device *d
> >> > 95 struct drm_device *
> >> > 216 struct drm_device *ddev
> >> > 234 struct drm_device *drm_dev
> >> > 611 struct drm_device *drm
> >> >4190 struct drm_device *dev
> >> >
> >> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> >> > it's not only me and others like the result of this effort it should be
> >> > followed up by adapting the other structs and the individual usages in
> >> > the different drivers.
> >>
> >> I think this is an unnecessary change. In drm, a dev is usually a drm
> >> device, i.e. struct drm_device *.
> >
> > Well, unless it's not. Prominently there is
> >
> > struct drm_device {
> > ...
> > struct device *dev;
> > ...
> > };
> >
> > which yields quite a few code locations using dev->dev which is
> > IMHO unnecessary irritating:
> >
> > $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> > 1633
> >
> > Also the functions that deal with both a struct device and a struct
> > drm_device often use "dev" for the struct device and then "ddev" for
> > the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
>
> Why is specifically struct drm_device *dev so irritating to you?
>
> You lead us to believe it's an outlier in kernel, something that goes
> against common kernel style, but it's really not:
>
> $ git grep -how "struct [A-Za-z0-9_]\+ \*dev" | sort | uniq -c | sort -rn |
> head -20
> 38494 struct device *dev
> 16388 struct net_device *dev
>4184 struct drm_device *dev
>2780 struct pci_dev *dev
>1916 struct comedi_device *dev
>1510 struct mlx5_core_dev *dev
>1057 struct mlx4_dev *dev
> 894 struct b43_wldev *dev
> 762 struct input_dev *dev
> 623 struct usbnet *dev
> 561 struct mlx5_ib_dev *dev
> 525 struct mt76_dev *dev
> 465 struct mt76x02_dev *dev
> 435 struct platform_device *dev
> 431 struct usb_device *dev
> 411 struct mt7915_dev *dev
> 398 struct cx231xx *dev
> 378 struct mei_device *dev
> 363 struct ksz_device *dev
> 359 struct mthca_dev *dev
>
> A good portion of the above also have a dev member.
Not all of them access both the foo_device and the device pointers.
Let's put the number of 435 platform_device pointers named "dev"
into perspective:
10095 struct platform_device *pdev
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Uwe,
Let's add some fuel to keep the thread alive ;-)
On Wed, Jul 12, 2023 at 6:13 PM Uwe Kleine-König
wrote:
> On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> > On Wed, 12 Jul 2023, Uwe Kleine-König
> > wrote:
> > > Hello,
> > >
> > > while I debugged an issue in the imx-lcdc driver I was constantly
> > > irritated about struct drm_device pointer variables being named "dev"
> > > because with that name I usually expect a struct device pointer.
> > >
> > > I think there is a big benefit when these are all renamed to "drm_dev".
> > > I have no strong preference here though, so "drmdev" or "drm" are fine
> > > for me, too. Let the bikesheding begin!
> > >
> > > Some statistics:
> > >
> > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq
> > > -c | sort -n
> > > 1 struct drm_device *adev_to_drm
> > > 1 struct drm_device *drm_
> > > 1 struct drm_device *drm_dev
> > > 1 struct drm_device*drm_dev
> > > 1 struct drm_device *pdev
> > > 1 struct drm_device *rdev
> > > 1 struct drm_device *vdev
> > > 2 struct drm_device *dcss_drv_dev_to_drm
> > > 2 struct drm_device **ddev
> > > 2 struct drm_device *drm_dev_alloc
> > > 2 struct drm_device *mock
> > > 2 struct drm_device *p_ddev
> > > 5 struct drm_device *device
> > > 9 struct drm_device * dev
> > > 25 struct drm_device *d
> > > 95 struct drm_device *
> > > 216 struct drm_device *ddev
> > > 234 struct drm_device *drm_dev
> > > 611 struct drm_device *drm
> > >4190 struct drm_device *dev
> > >
> > > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > > it's not only me and others like the result of this effort it should be
> > > followed up by adapting the other structs and the individual usages in
> > > the different drivers.
> >
> > I think this is an unnecessary change. In drm, a dev is usually a drm
> > device, i.e. struct drm_device *.
>
> Well, unless it's not. Prominently there is
>
> struct drm_device {
> ...
> struct device *dev;
> ...
> };
>
> which yields quite a few code locations using dev->dev which is
> IMHO unnecessary irritating:
>
> $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> 1633
I find that irritating as well...
Same for e.g. crtc->crtc.
Hence that's why I had sent patches to rename the base members in the
shmob_drm-specific subclasses of drm_{crtc,connector,plane} to "base".
https://lore.kernel.org/dri-devel/b3daca80f82625ba14e3aeaf2fca6dcefa056e47.1687423204.git.geert+rene...@glider.be
> Also the functions that deal with both a struct device and a struct
> drm_device often use "dev" for the struct device and then "ddev" for
> the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
I guess you considered "drm_dev", because it is still a short name?
Code dealing with platform devices usually uses "pdev" and "dev".
Same for PCI drivers (despite "pci_dev" being a short name).
So my personal preference goes to "ddev".
EOF (End-of-Fuel ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v5 24/33] m68k: Convert various functions to use ptdescs
Hi Vishal,
On Thu, Jun 22, 2023 at 10:58 PM Vishal Moola (Oracle)
wrote:
> As part of the conversions to replace pgtable constructor/destructors with
> ptdesc equivalents, convert various page table functions to use ptdescs.
>
> Some of the functions use the *get*page*() helper functions. Convert
> these to use pagetable_alloc() and ptdesc_address() instead to help
> standardize page tables further.
>
> Signed-off-by: Vishal Moola (Oracle)
Thanks for your patch!
> --- a/arch/m68k/include/asm/mcf_pgalloc.h
> +++ b/arch/m68k/include/asm/mcf_pgalloc.h
> static inline pgd_t *pgd_alloc(struct mm_struct *mm)
> {
> pgd_t *new_pgd;
> + struct ptdesc *ptdesc = pagetable_alloc((GFP_DMA | GFP_NOWARN) &
0-day already told you for v3 that GFP_NOWARN does not exist.
Please try cross-compiling your changes:
https://mirrors.edge.kernel.org/pub/tools/crosstool/
> + ~__GFP_HIGHMEM, 0);
>
> - new_pgd = (pgd_t *)__get_free_page(GFP_DMA | __GFP_NOWARN);
> - if (!new_pgd)
> + if (!ptdesc)
> return NULL;
> + new_pgd = ptdesc_address(ptdesc);
> +
> memcpy(new_pgd, swapper_pg_dir, PTRS_PER_PGD * sizeof(pgd_t));
> memset(new_pgd, 0, PAGE_OFFSET >> PGDIR_SHIFT);
> return new_pgd;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v4 27/34] nios2: Convert __pte_free_tlb() to use ptdescs
Hi Dinh,
On Wed, Jun 14, 2023 at 12:17 AM Dinh Nguyen wrote:
> On 6/12/23 16:04, Vishal Moola (Oracle) wrote:
> > Part of the conversions to replace pgtable constructor/destructors with
> > ptdesc equivalents.
> >
> > Signed-off-by: Vishal Moola (Oracle)
> > ---
> > arch/nios2/include/asm/pgalloc.h | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/nios2/include/asm/pgalloc.h
> > b/arch/nios2/include/asm/pgalloc.h
> > index ecd1657bb2ce..ce6bb8e74271 100644
> > --- a/arch/nios2/include/asm/pgalloc.h
> > +++ b/arch/nios2/include/asm/pgalloc.h
> > @@ -28,10 +28,10 @@ static inline void pmd_populate(struct mm_struct *mm,
> > pmd_t *pmd,
> >
> > extern pgd_t *pgd_alloc(struct mm_struct *mm);
> >
> > -#define __pte_free_tlb(tlb, pte, addr) \
> > - do {\
> > - pgtable_pte_page_dtor(pte); \
> > - tlb_remove_page((tlb), (pte)); \
> > +#define __pte_free_tlb(tlb, pte, addr)
> > \
> > + do {\
> > + pagetable_pte_dtor(page_ptdesc(pte)); \
> > + tlb_remove_page_ptdesc((tlb), (page_ptdesc(pte))); \
> > } while (0)
> >
> > #endif /* _ASM_NIOS2_PGALLOC_H */
>
> Applied!
I don't think you can just apply this patch, as the new functions
were only introduced in [PATCH v4 05/34] of this series.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v4 25/34] m68k: Convert various functions to use ptdescs
On Mon, Jun 12, 2023 at 11:05 PM Vishal Moola (Oracle)
wrote:
> As part of the conversions to replace pgtable constructor/destructors with
> ptdesc equivalents, convert various page table functions to use ptdescs.
>
> Some of the functions use the *get*page*() helper functions. Convert
> these to use pagetable_alloc() and ptdesc_address() instead to help
> standardize page tables further.
>
> Signed-off-by: Vishal Moola (Oracle)
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v3 30/34] sh: Convert pte_free_tlb() to use ptdescs
Hi Adrian,
On Thu, Jun 1, 2023 at 9:28 AM John Paul Adrian Glaubitz
wrote:
> On Thu, 2023-06-01 at 09:20 +0200, Geert Uytterhoeven wrote:
> > On Wed, May 31, 2023 at 11:33 PM Vishal Moola (Oracle)
> > wrote:
> > > Part of the conversions to replace pgtable constructor/destructors with
> > > ptdesc equivalents. Also cleans up some spacing issues.
> > >
> > > Signed-off-by: Vishal Moola (Oracle)
> >
> > LGTM, so
> > Reviewed-by: Geert Uytterhoeven
>
> I assume this series is supposed to go through some mm tree?
I think so, so your Acked-by would be appreciated...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v3 25/34] m68k: Convert various functions to use ptdescs
Hi Vishal,
On Wed, May 31, 2023 at 11:32 PM Vishal Moola (Oracle)
wrote:
> As part of the conversions to replace pgtable constructor/destructors with
> ptdesc equivalents, convert various page table functions to use ptdescs.
>
> Some of the functions use the *get*page*() helper functions. Convert
> these to use pagetable_alloc() and ptdesc_address() instead to help
> standardize page tables further.
>
> Signed-off-by: Vishal Moola (Oracle)
Thanks for your patch!
> --- a/arch/m68k/include/asm/mcf_pgalloc.h
> +++ b/arch/m68k/include/asm/mcf_pgalloc.h
> @@ -7,20 +7,19 @@
>
> extern inline void pte_free_kernel(struct mm_struct *mm, pte_t *pte)
> {
> - free_page((unsigned long) pte);
> + pagetable_free(virt_to_ptdesc(pte));
> }
>
> extern const char bad_pmd_string[];
>
> extern inline pte_t *pte_alloc_one_kernel(struct mm_struct *mm)
> {
> - unsigned long page = __get_free_page(GFP_DMA);
> + struct ptdesc *ptdesc = pagetable_alloc(GFP_DMA | __GFP_ZERO, 0);
>
> - if (!page)
> + if (!ptdesc)
> return NULL;
>
> - memset((void *)page, 0, PAGE_SIZE);
> - return (pte_t *) (page);
> + return (pte_t *) (ptdesc_address(ptdesc));
No need to cast "void *" when returning a different pointer type.
> }
>
> extern inline pmd_t *pmd_alloc_kernel(pgd_t *pgd, unsigned long address)
> @@ -35,36 +34,36 @@ extern inline pmd_t *pmd_alloc_kernel(pgd_t *pgd,
> unsigned long address)
> static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t pgtable,
> unsigned long address)
> {
> - struct page *page = virt_to_page(pgtable);
> + struct ptdesc *ptdesc = virt_to_ptdesc(pgtable);
>
> - pgtable_pte_page_dtor(page);
> - __free_page(page);
> + pagetable_pte_dtor(ptdesc);
> + pagetable_free(ptdesc);
> }
>
> static inline pgtable_t pte_alloc_one(struct mm_struct *mm)
> {
> - struct page *page = alloc_pages(GFP_DMA, 0);
> + struct ptdesc *ptdesc = pagetable_alloc(GFP_DMA, 0);
> pte_t *pte;
>
> - if (!page)
> + if (!ptdesc)
> return NULL;
> - if (!pgtable_pte_page_ctor(page)) {
> - __free_page(page);
> + if (!pagetable_pte_ctor(ptdesc)) {
> + pagetable_free(ptdesc);
> return NULL;
> }
>
> - pte = page_address(page);
> - clear_page(pte);
> + pte = ptdesc_address(ptdesc);
> + pagetable_clear(pte);
>
> return pte;
> }
>
> static inline void pte_free(struct mm_struct *mm, pgtable_t pgtable)
> {
> - struct page *page = virt_to_page(pgtable);
> + struct ptdesc *ptdesc = virt_to_ptdesc(ptdesc);
virt_to_ptdesc(pgtable)
(You can build this using m5475evb_defconfig)
>
> - pgtable_pte_page_dtor(page);
> - __free_page(page);
> + pagetable_pte_dtor(ptdesc);
> + pagetable_free(ptdesc);
> }
>
> /*
> @@ -75,16 +74,18 @@ static inline void pte_free(struct mm_struct *mm,
> pgtable_t pgtable)
>
> static inline void pgd_free(struct mm_struct *mm, pgd_t *pgd)
> {
> - free_page((unsigned long) pgd);
> + pagetable_free(virt_to_ptdesc(pgd));
> }
>
> static inline pgd_t *pgd_alloc(struct mm_struct *mm)
> {
> pgd_t *new_pgd;
> + struct ptdesc *ptdesc = pagetable_alloc(GFP_DMA | GFP_NOWARN, 0);
>
> - new_pgd = (pgd_t *)__get_free_page(GFP_DMA | __GFP_NOWARN);
> - if (!new_pgd)
> + if (!ptdesc)
> return NULL;
> + new_pgd = (pgd_t *) ptdesc_address(ptdesc);
No need to cast "void *" when assigning to a different pointer type.
> +
> memcpy(new_pgd, swapper_pg_dir, PTRS_PER_PGD * sizeof(pgd_t));
> memset(new_pgd, 0, PAGE_OFFSET >> PGDIR_SHIFT);
> return new_pgd;
The rest LGTM.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v3 30/34] sh: Convert pte_free_tlb() to use ptdescs
On Wed, May 31, 2023 at 11:33 PM Vishal Moola (Oracle)
wrote:
> Part of the conversions to replace pgtable constructor/destructors with
> ptdesc equivalents. Also cleans up some spacing issues.
>
> Signed-off-by: Vishal Moola (Oracle)
LGTM, so
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()
Hi Andy,
On Thu, Mar 23, 2023 at 4:15 PM Andy Shevchenko
wrote:
> On Thu, Mar 23, 2023 at 10:02:38AM -0500, Bjorn Helgaas wrote:
> > I poked around looking for similar patterns elsewhere with:
> > git grep "#define.*for_each_.*_p("
> > git grep "#define.*for_each_.*_idx("
> >
> > I didn't find any other "_p" iterators and just a few "_idx" ones, so
> > my hope is to follow what little precedent there is, as well as
> > converge on the basic "*_for_each_resource()" iterators and remove the
> > "_idx()" versions over time by doing things like the
> > pci_claim_resource() change.
>
> The p is heavily used in the byte order conversion helpers.
I can't seem to find them. Example?
Or do you mean cpu_to_be32p()? There "p" means pointer,
which is something completely different.
> > What do you think? If it seems like excessive churn, we can do it
> > as-is and still try to reduce the use of the index variable over time.
>
> I think _p has a precedent as well. But I can think about it a bit, maybe
> we can come up with something smarter.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 19/36] objtool/idle: Validate __cpuidle code as noinstr
On Wed, Jun 8, 2022 at 4:46 PM Peter Zijlstra wrote:
> Idle code is very like entry code in that RCU isn't available. As
> such, add a little validation.
>
> Signed-off-by: Peter Zijlstra (Intel)
> arch/m68k/kernel/vmlinux-nommu.lds |1 -
> arch/m68k/kernel/vmlinux-std.lds |1 -
> arch/m68k/kernel/vmlinux-sun3.lds| 1 -
FWIW
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v8 16/27] m68k: Switch to new sys-off handler API
Hi Dmitry,
On Tue, May 10, 2022 at 1:34 AM Dmitry Osipenko
wrote:
> Kernel now supports chained power-off handlers. Use
> register_power_off_handler() that registers power-off handlers and
> do_kernel_power_off() that invokes chained power-off handlers. Legacy
> pm_power_off() will be removed once all drivers will be converted to
> the new sys-off API.
>
> Normally arch code should adopt only the do_kernel_power_off() at first,
> but m68k is a special case because it uses pm_power_off() "inside out",
> i.e. pm_power_off() invokes machine_power_off() [in fact it does nothing],
> while it's machine_power_off() that should invoke the pm_power_off(), and
> thus, we can't convert platforms to the new API separately. There are only
> two platforms changed here, so it's not a big deal.
>
> Acked-by: Geert Uytterhoeven
> Reviewed-by: Michał Mirosław
> Signed-off-by: Dmitry Osipenko
Thanks for your patch, which is now commit f0f7e5265b3b37b0
("m68k: Switch to new sys-off handler API") upstream.
> --- a/arch/m68k/emu/natfeat.c
> +++ b/arch/m68k/emu/natfeat.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -90,5 +91,5 @@ void __init nf_init(void)
> pr_info("NatFeats found (%s, %lu.%lu)\n", buf, version >> 16,
> version & 0x);
>
> - mach_power_off = nf_poweroff;
> + register_platform_power_off(nf_poweroff);
Unfortunately nothing is registered, as this is called very early
(from setup_arch(), before the memory allocator is available.
Hence register_sys_off_handler() fails with -ENOMEM, and poweroff
stops working.
Possible solutions:
- As at most one handler can be registered,
register_platform_power_off() could use a static struct sys_off_handler
instance,
- Keep mach_power_off, and call register_platform_power_off() later.
Anything else?
Thanks!
> --- a/arch/m68k/mac/config.c
> +++ b/arch/m68k/mac/config.c
> @@ -12,6 +12,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -140,7 +141,6 @@ void __init config_mac(void)
> mach_hwclk = mac_hwclk;
> mach_reset = mac_reset;
> mach_halt = mac_poweroff;
> - mach_power_off = mac_poweroff;
> #if IS_ENABLED(CONFIG_INPUT_M68K_BEEP)
> mach_beep = mac_mksound;
> #endif
> @@ -160,6 +160,8 @@ void __init config_mac(void)
>
> if (macintosh_config->ident == MAC_MODEL_IICI)
> mach_l2_flush = via_l2_flush;
> +
> + register_platform_power_off(mac_poweroff);
> }
This must have the same problem.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v8 07/27] kernel/reboot: Add kernel_can_power_off()
Hi Dmitry,
On Tue, May 24, 2022 at 3:41 PM Dmitry Osipenko
wrote:
> On 5/24/22 16:14, Geert Uytterhoeven wrote:
> > On Tue, May 10, 2022 at 1:33 AM Dmitry Osipenko
> > wrote:
> >> Add kernel_can_power_off() helper that replaces open-coded checks of
> >> the global pm_power_off variable. This is a necessary step towards
> >> supporting chained power-off handlers.
> >>
> >> Signed-off-by: Dmitry Osipenko
> >
> > Thanks for your patch, which is now commit 0e2110d2e910e44c
> > ("kernel/reboot: Add kernel_can_power_off()") in pm/linux-next.
> >
> > This causes the "poweroff" command (Debian nfsroot) to no longer
> > cleanly halt the system on arm32 systems, but fail with a panic
> > instead:
> >
> > -reboot: System halted
> > +reboot: Power down
> > +Kernel panic - not syncing: Attempted to kill init! exitcode=0x
> > +CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted
> > 5.18.0-rc7-shmobile-7-g0e2110d2e910 #1274
> > +Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> > + unwind_backtrace from show_stack+0x10/0x14
> > + show_stack from dump_stack_lvl+0x40/0x4c
> > + dump_stack_lvl from panic+0xf4/0x330
> > + panic from do_exit+0x1c8/0x8e4
> > + do_exit from __do_sys_reboot+0x174/0x1fc
> > + __do_sys_reboot from ret_fast_syscall+0x0/0x54
> > +Exception stack(0xf0815fa8 to 0xf0815ff0)
> > +5fa0: 004e6954 fee1dead 28121969 4321fedc
> > f0d94600
> > +5fc0: 004e6954 0058 befa0c78 befa0c10
> > 004e56f8
> > +5fe0: 0058 befa0b6c b6ec8d45 b6e4a746
> > +---[ end Kernel panic - not syncing: Attempted to kill init!
> > exitcode=0x ]---
> >
> > On arm64, "poweroff" causes a clean "reboot: Power down" before/after.
> >
> > On both arm32 and arm64, the same handlers are registered:
> > - SYS_OFF_MODE_POWER_OFF_PREPARE: legacy_pm_power_off_prepare
> > - SYS_OFF_MODE_POWER_OFF: legacy_pm_power_off
> >
> > On both arm32 and arm64, legacy_pm_power_off_prepare() is called.
> > On both arm32 and arm64, legacy_pm_power_off() does not seem to
> > be called.
> >
> > On arm32, both pm_power_off_prepare and pm_power_off are NULL.
> > On arm64, pm_power_off_prepare is NULL, and
> > pm_power_off is psci_sys_poweroff.
> >
> > Do you have a clue?
> > Thanks!
>
> Thank you, Geert! I see the problem, the kernel_can_power_off() checks
> whether power-off handler is registered, but it's always registered because
> legacy_pm_power_off is registered unconditionally. So it causes trouble for
> platforms that don't have power-off handler installed at all. All platforms
> that I tested have a power-off handler, so now wonder that I didn't notice
> this before.
>
> This change should fix the problem, please give it a try:
Thank you, that fixes the problem for me!
Tested-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v8 07/27] kernel/reboot: Add kernel_can_power_off()
Hi Dmitry,
On Tue, May 10, 2022 at 1:33 AM Dmitry Osipenko
wrote:
> Add kernel_can_power_off() helper that replaces open-coded checks of
> the global pm_power_off variable. This is a necessary step towards
> supporting chained power-off handlers.
>
> Signed-off-by: Dmitry Osipenko
Thanks for your patch, which is now commit 0e2110d2e910e44c
("kernel/reboot: Add kernel_can_power_off()") in pm/linux-next.
This causes the "poweroff" command (Debian nfsroot) to no longer
cleanly halt the system on arm32 systems, but fail with a panic
instead:
-reboot: System halted
+reboot: Power down
+Kernel panic - not syncing: Attempted to kill init! exitcode=0x
+CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted
5.18.0-rc7-shmobile-7-g0e2110d2e910 #1274
+Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
+ unwind_backtrace from show_stack+0x10/0x14
+ show_stack from dump_stack_lvl+0x40/0x4c
+ dump_stack_lvl from panic+0xf4/0x330
+ panic from do_exit+0x1c8/0x8e4
+ do_exit from __do_sys_reboot+0x174/0x1fc
+ __do_sys_reboot from ret_fast_syscall+0x0/0x54
+Exception stack(0xf0815fa8 to 0xf0815ff0)
+5fa0: 004e6954 fee1dead 28121969 4321fedc f0d94600
+5fc0: 004e6954 0058 befa0c78 befa0c10 004e56f8
+5fe0: 0058 befa0b6c b6ec8d45 b6e4a746
+---[ end Kernel panic - not syncing: Attempted to kill init!
exitcode=0x ]---
On arm64, "poweroff" causes a clean "reboot: Power down" before/after.
On both arm32 and arm64, the same handlers are registered:
- SYS_OFF_MODE_POWER_OFF_PREPARE: legacy_pm_power_off_prepare
- SYS_OFF_MODE_POWER_OFF: legacy_pm_power_off
On both arm32 and arm64, legacy_pm_power_off_prepare() is called.
On both arm32 and arm64, legacy_pm_power_off() does not seem to
be called.
On arm32, both pm_power_off_prepare and pm_power_off are NULL.
On arm64, pm_power_off_prepare is NULL, and
pm_power_off is psci_sys_poweroff.
Do you have a clue?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v8 00/27] Introduce power-off+restart call chain API
Hi Rafael,
On Wed, May 18, 2022 at 4:46 PM Rafael J. Wysocki wrote:
> On Tue, May 10, 2022 at 1:33 AM Dmitry Osipenko
> wrote:
> > m68k: Switch to new sys-off handler API
Sorry, I didn't realize this was going to interact with the new m68k
virtual machine support, which is included in the m68k pull request
for v5.19.
> However, I'm going to send a pull request with it in the second half
> of the merge window, after the majority of the other changes in the
> subsystems touched by it have been integrated.
And presumably you will have to merge in v5.19-rc1, too?
I've sent a fix. It should appear at
https://lore.kernel.org/r/20220523175520.949681-1-ge...@linux-m68k.org
soon.
Can you please include that in your PR?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v7 00/20] Introduce power-off+restart call chain API
Hi Dmitry,
On Tue, Apr 12, 2022 at 1:38 AM Dmitry Osipenko
wrote:
> Problem
> ---
>
> SoC devices require power-off call chaining functionality from kernel.
> We have a widely used restart chaining provided by restart notifier API,
> but nothing for power-off.
> Changelog:
>
> v7: - Rebased on a recent linux-next. Dropped the recently removed
> NDS32 architecture. Only SH and x86 arches left un-acked.
>
> - Added acks from Thomas Bogendoerfer and Krzysztof Kozlowski
> to the MIPS and memory/emif patches respectively.
Looks like you forgot to add the actual acks?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
On Fri, Dec 10, 2021 at 8:14 PM Rafael J. Wysocki wrote:
> On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko wrote:
> > 10.12.2021 21:27, Rafael J. Wysocki пишет:
> > > On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko wrote:
> > >> 29.11.2021 03:26, Michał Mirosław пишет:
> > >>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> > >>>> 28.11.2021 03:28, Michał Mirosław пишет:
> > >>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> > >>>>>> Add sanity check which ensures that there are no two restart handlers
> > >>>>>> registered with the same priority. Normally it's a direct sign of a
> > >>>>>> problem if two handlers use the same priority.
> > >>>>>
> > >>>>> The patch doesn't ensure the property that there are no
> > >>>>> duplicated-priority
> > >>>>> entries on the chain.
> > >>>>
> > >>>> It's not the exact point of this patch.
> > >>>>
> > >>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
> > >>>>> -EBUSY or something istead of adding an entry with duplicate priority.
> > >>>>> That way it would need only one list traversal unless you want to
> > >>>>> register the duplicate anyway (then you would call the older
> > >>>>> atomic_notifier_chain_register() after reporting the error).
> > >>>>
> > >>>> The point of this patch is to warn developers about the problem that
> > >>>> needs to be fixed. We already have such troubling drivers in mainline.
> > >>>>
> > >>>> It's not critical to register different handlers with a duplicated
> > >>>> priorities, but such cases really need to be corrected. We shouldn't
> > >>>> break users' machines during transition to the new API, meanwhile
> > >>>> developers should take action of fixing theirs drivers.
> > >>>>
> > >>>>> (Or you could return > 0 when a duplicate is registered in
> > >>>>> atomic_notifier_chain_register() if the callers are prepared
> > >>>>> for that. I don't really like this way, though.)
> > >>>>
> > >>>> I had a similar thought at some point before and decided that I'm not
> > >>>> in
> > >>>> favor of this approach. It's nicer to have a dedicated function that
> > >>>> verifies the uniqueness, IMO.
> > >>>
> > >>> I don't like the part that it traverses the list second time to check
> > >>> the uniqueness. But actually you could avoid that if
> > >>> notifier_chain_register() would always add equal-priority entries in
> > >>> reverse order:
> > >>>
> > >>> static int notifier_chain_register(struct notifier_block **nl,
> > >>> struct notifier_block *n)
> > >>> {
> > >>> while ((*nl) != NULL) {
> > >>> if (unlikely((*nl) == n)) {
> > >>> WARN(1, "double register detected");
> > >>> return 0;
> > >>> }
> > >>> - if (n->priority > (*nl)->priority)
> > >>> + if (n->priority >= (*nl)->priority)
> > >>> break;
> > >>> nl = &((*nl)->next);
> > >>> }
> > >>> n->next = *nl;
> > >>> rcu_assign_pointer(*nl, n);
> > >>> return 0;
> > >>> }
> > >>>
> > >>> Then the check for uniqueness after adding would be:
> > >>>
> > >>> WARN(nb->next && nb->priority == nb->next->priority);
> > >>
> > >> We can't just change the registration order because invocation order of
> > >> the call chain depends on the registration order
> > >
> > > It doesn't if unique priorities are required and isn't that what you want?
> > >
> > >> and some of current
> > >> users may rely on that order. I'm pretty sure that changing the order
> > >> will have unfortunate consequences.
> > >
> > > Well, the WARN() doesn't help much then.
> > >
> > > Either you can make all of the users register with unique priorities,
> > > and then you can make the registration reject non-unique ones, or you
> > > cannot assume them to be unique.
> >
> > There is no strong requirement for priorities to be unique, the reboot.c
> > code will work properly.
>
> In which case adding the WARN() is not appropriate IMV.
>
> Also I've looked at the existing code and at least in some cases the
> order in which the notifiers run doesn't matter. I'm not sure what
> the purpose of this patch is TBH.
>
> > The potential problem is on the user's side and the warning is intended
> > to aid the user.
>
> Unless somebody has the panic_on_warn mentioned previously set and
> really the user need not understand what the WARN() is about. IOW,
> WARN() helps developers, not users.
Do panic_on_warn and reboot_on_panic play well with having a WARN()
in the reboot notifier handling?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] xen-pciback: allow compiling on other archs than x86
Hi Oleksandr,
On Thu, Oct 28, 2021 at 8:15 AM Oleksandr Andrushchenko
wrote:
> From: Oleksandr Andrushchenko
>
> Xen-pciback driver was designed to be built for x86 only. But it
> can also be used by other architectures, e.g. Arm.
>
> Currently PCI backend implements multiple functionalities at a time,
> such as:
> 1. It is used as a database for assignable PCI devices, e.g. xl
>pci-assignable-{add|remove|list} manipulates that list. So, whenever
>the toolstack needs to know which PCI devices can be passed through
>it reads that from the relevant sysfs entries of the pciback.
> 2. It is used to hold the unbound PCI devices list, e.g. when passing
>through a PCI device it needs to be unbound from the relevant device
>driver and bound to pciback (strictly speaking it is not required
>that the device is bound to pciback, but pciback is again used as a
>database of the passed through PCI devices, so we can re-bind the
>devices back to their original drivers when guest domain shuts down)
> 3. Device reset for the devices being passed through
> 4. Para-virtualised use-cases support
>
> The para-virtualised part of the driver is not always needed as some
> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
> model for PCI device passthrough.
>
> For such use-cases make the very first step in splitting the
> xen-pciback driver into two parts: Xen PCI stub and PCI PV backend
> drivers.
>
> For that add new configuration options CONFIG_XEN_PCI_STUB and
> CONFIG_XEN_PCIDEV_STUB, so the driver can be limited in its
> functionality, e.g. no support for para-virtualised scenario.
> x86 platform will continue using CONFIG_XEN_PCIDEV_BACKEND for the
> fully featured backend driver.
>
> Signed-off-by: Oleksandr Andrushchenko
> Signed-off-by: Anastasiia Lukianenko
> Reviewed-by: Stefano Stabellini
> Reviewed-by: Juergen Gross
Thanks for your patch, which is now commit a67efff28832a597
("xen-pciback: allow compiling on other archs than x86")
in v5.16-rc1.
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -181,10 +181,34 @@ config SWIOTLB_XEN
> select DMA_OPS
> select SWIOTLB
>
> +config XEN_PCI_STUB
> + bool
> +
> +config XEN_PCIDEV_STUB
> + tristate "Xen PCI-device stub driver"
> + depends on PCI && !X86 && XEN
> + depends on XEN_BACKEND
> + select XEN_PCI_STUB
> + default m
Please note that this means "default y" if CONFIG_MODULES=n.
Perhaps this should be "default m if MODULES" instead?
> + help
> + The PCI device stub driver provides limited version of the PCI
> + device backend driver without para-virtualized support for guests.
> + If you select this to be a module, you will need to make sure no
> + other driver has bound to the device(s) you want to make visible to
> + other guests.
> +
> + The "hide" parameter (only applicable if backend driver is compiled
> + into the kernel) allows you to bind the PCI devices to this module
> + from the default device drivers. The argument is the list of PCI
> BDFs:
> + xen-pciback.hide=(03:00.0)(04:00.0)
> +
> + If in doubt, say m.
> +
> config XEN_PCIDEV_BACKEND
> tristate "Xen PCI-device backend driver"
> depends on PCI && X86 && XEN
> depends on XEN_BACKEND
> + select XEN_PCI_STUB
> default m
> help
> The PCI device backend driver allows the kernel to export arbitrary
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v0 42/42] notifier: Return an error when callback is already registered
Hi Borislav,
On Mon, Nov 8, 2021 at 4:59 PM Borislav Petkov wrote:
> On Mon, Nov 08, 2021 at 04:25:47PM +0100, Geert Uytterhoeven wrote:
> > I'm not against returning proper errors codes. I'm against forcing
> > callers to check things that cannot fail and to add individual error
> > printing to each and every caller.
>
> If you're against checking things at the callers, then the registration
> function should be void. IOW, those APIs are not optimally designed atm.
Returning void is the other extreme ;-)
There are 3 levels (ignoring BUG_ON()/panic () inside the callee):
1. Return void: no one can check success or failure,
2. Return an error code: up to the caller to decide,
3. Return a __must_check error code: every caller must check.
I'm in favor of 2, as there are several places where it cannot fail.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v0 42/42] notifier: Return an error when callback is already registered
Hi Borislav,
On Mon, Nov 8, 2021 at 3:21 PM Borislav Petkov wrote:
> On Mon, Nov 08, 2021 at 03:07:03PM +0100, Geert Uytterhoeven wrote:
> > I think the addition of __must_check is overkill, leading to the
> > addition of useless error checks and message printing.
>
> See the WARN in notifier_chain_register() - it will already do "message
> printing".
I mean the addition of useless error checks and message printing _to
the callers_.
> > Many callers call this where it cannot fail, and where nothing can
> > be done in the very unlikely event that the call would ever start to
> > fail.
>
> This is an attempt to remove this WARN() hack in
> notifier_chain_register() and have the function return a proper error
> value instead of this "Currently always returns zero." which is bad
> design.
>
> Some of the registration functions around the tree check that retval and
> some don't. So if "it cannot fail" those registration either should not
> return a value or callers should check that return value - what we have
> now doesn't make a whole lot of sense.
With __must_check callers are required to check, even if they know
it cannot fail.
> Oh, and then fixing this should avoid stuff like:
>
> + if (notifier_registered == false) {
> + mce_register_decode_chain(_bad_page_nb);
> + notifier_registered = true;
> + }
>
> from propagating in the code.
That's unrelated to the addition of __must_check.
I'm not against returning proper errors codes. I'm against forcing
callers to check things that cannot fail and to add individual error
printing to each and every caller.
Note that in other areas, we are moving in the other
direction, to a centralized printing of error messages,
cfr. e.g. commit 7723f4c5ecdb8d83 ("driver core: platform: Add an
error message to platform_get_irq*()").
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v0 42/42] notifier: Return an error when callback is already registered
Hi Borislav,
On Mon, Nov 8, 2021 at 11:13 AM Borislav Petkov wrote:
> From: Borislav Petkov
>
> The notifier registration routine doesn't return a proper error value
> when a callback has already been registered, leading people to track
> whether that registration has happened at the call site:
>
> https://lore.kernel.org/amd-gfx/20210512013058.6827-1-mukul.jo...@amd.com/
>
> Which is unnecessary.
>
> Return -EEXIST to signal that case so that callers can act accordingly.
> Enforce callers to check the return value, leading to loud screaming
> during build:
>
> arch/x86/kernel/cpu/mce/core.c: In function ‘mce_register_decode_chain’:
> arch/x86/kernel/cpu/mce/core.c:167:2: error: ignoring return value of \
>‘blocking_notifier_chain_register’, declared with attribute
> warn_unused_result [-Werror=unused-result]
> blocking_notifier_chain_register(_mce_decoder_chain, nb);
> ^~~~
>
> Drop the WARN too, while at it.
>
> Suggested-by: Thomas Gleixner
> Signed-off-by: Borislav Petkov
Thanks for your patch!
> --- a/include/linux/notifier.h
> +++ b/include/linux/notifier.h
> @@ -141,13 +141,13 @@ extern void srcu_init_notifier_head(struct
> srcu_notifier_head *nh);
>
> #ifdef __KERNEL__
>
> -extern int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
> +extern int __must_check atomic_notifier_chain_register(struct
> atomic_notifier_head *nh,
> struct notifier_block *nb);
> -extern int blocking_notifier_chain_register(struct blocking_notifier_head
> *nh,
> +extern int __must_check blocking_notifier_chain_register(struct
> blocking_notifier_head *nh,
> struct notifier_block *nb);
> -extern int raw_notifier_chain_register(struct raw_notifier_head *nh,
> +extern int __must_check raw_notifier_chain_register(struct raw_notifier_head
> *nh,
> struct notifier_block *nb);
> -extern int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
> +extern int __must_check srcu_notifier_chain_register(struct
> srcu_notifier_head *nh,
> struct notifier_block *nb);
I think the addition of __must_check is overkill, leading to the
addition of useless error checks and message printing. Many callers
call this where it cannot fail, and where nothing can be done in the
very unlikely event that the call would ever start to fail.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v3 21/25] m68k: Switch to new sys-off handler API
On Mon, Nov 8, 2021 at 1:48 AM Dmitry Osipenko wrote:
> Kernel now supports chained power-off handlers. Use
> register_power_off_handler() that registers power-off handlers and
> do_kernel_power_off() that invokes chained power-off handlers. Legacy
> pm_power_off() will be removed once all drivers will be converted to
> the new power-off API.
>
> Normally arch code should adopt only the do_kernel_power_off() at first,
> but m68k is a special case because it uses pm_power_off() "inside out",
> i.e. pm_power_off() invokes machine_power_off() [in fact it does nothing],
> while it's machine_power_off() that should invoke the pm_power_off(), and
> thus, we can't convert platforms to the new API separately. There are only
> two platforms changed here, so it's not a big deal.
>
> Signed-off-by: Dmitry Osipenko
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v4 5/5] bus: Make remove callback return void
On Tue, Jul 13, 2021 at 9:35 PM Uwe Kleine-König
wrote:
> The driver core ignores the return value of this callback because there
> is only little it can do when a device disappears.
>
> This is the final bit of a long lasting cleanup quest where several
> buses were converted to also return void from their remove callback.
> Additionally some resource leaks were fixed that were caused by drivers
> returning an error code in the expectation that the driver won't go
> away.
>
> With struct bus_type::remove returning void it's prevented that newly
> implemented buses return an ignored error code and so don't anticipate
> wrong expectations for driver authors.
> drivers/zorro/zorro-driver.c | 3 +--
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 000/141] Fix fall-through warnings for Clang
Hi Miguel,
On Thu, Nov 26, 2020 at 3:54 PM Miguel Ojeda
wrote:
> On Wed, Nov 25, 2020 at 11:44 PM Edward Cree wrote:
> > To make the intent clear, you have to first be certain that you
> > understand the intent; otherwise by adding either a break or a
> > fallthrough to suppress the warning you are just destroying the
> > information that "the intent of this code is unknown".
>
> If you don't know what the intent of your own code is, then you
> *already* have a problem in your hands.
The maintainer is not necessarily the owner/author of the code, and
thus may not know the intent of the code.
> > or does it flag up code
> > that can be mindlessly "fixed" (in which case the warning is
> > worthless)? Proponents in this thread seem to be trying to
> > have it both ways.
>
> A warning is not worthless just because you can mindlessly fix it.
> There are many counterexamples, e.g. many
> checkpatch/lint/lang-format/indentation warnings, functional ones like
> the `if (a = b)` warning...
BTW, you cannot mindlessly fix the latter, as you cannot know if
"(a == b)" or "((a = b))" was intended, without understanding the code
(and the (possibly unavailable) data sheet, and the hardware, ...).
P.S. So far I've stayed out of this thread, as I like it if the compiler
flags possible mistakes. After all I was the one fixing new
"may be used uninitialized" warnings thrown up by gcc-4.1, until
(a bit later than) support for that compiler was removed...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [Xen-devel] [RESEND TRIVIAL 3/3] treewide: arch: Fix Kconfig indentation
On Fri, Oct 4, 2019 at 4:57 PM Krzysztof Kozlowski wrote:
> Adjust indentation from spaces to tab (+optional two spaces) as in
> coding style with command like:
> $ sed -e 's/^/\t/' -i */Kconfig
>
> Signed-off-by: Krzysztof Kozlowski
> arch/m68k/Kconfig.bus | 2 +-
> arch/m68k/Kconfig.debug| 16
> arch/m68k/Kconfig.machine | 8 ----
For m68k:
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 0/2] Remove support for deprecated %pf and %pF in vsprintf
Hi Sakari,
On Fri, Mar 22, 2019 at 2:25 PM Sakari Ailus
wrote:
> The printk family of functions supports %ps and %pS conversion specifiers
> to print function names. Yet the deprecated %pf and %pF conversion
> specifiers with equivalent functionality remain supported. A number of
> users of %pf and %pF remain.
>
> This patchsets converts the existing users of %pf and %pF to %ps and %pS,
> respectively, and removes support for the deprecated %pf and %pF.
>
> The patches apply cleanly both on 5.1-rc1 as well as on Linux-next. No new
> %pf or %pF users have been added in the meantime so the patch is
> sufficient as itself on linux-next, too.
Do you know in which commit they became deprecated, so the backporters
know how far this can be backported safely?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
On Mon, Jan 21, 2019 at 9:06 AM Mike Rapoport wrote:
> Add check for the return value of memblock_alloc*() functions and call
> panic() in case of error.
> The panic message repeats the one used by panicing memblock allocators with
> adjustment of parameters to include only relevant ones.
>
> The replacement was mostly automated with semantic patches like the one
> below with manual massaging of format strings.
>
> @@
> expression ptr, size, align;
> @@
> ptr = memblock_alloc(size, align);
> + if (!ptr)
> + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
> size, align);
>
> Signed-off-by: Mike Rapoport
> arch/m68k/atari/stram.c | 4
> arch/m68k/mm/init.c | 3 +++
> arch/m68k/mm/mcfmmu.c | 6 ++
> arch/m68k/mm/motorola.c | 9 +
> arch/m68k/mm/sun3mmu.c| 6 ++
> arch/m68k/sun3/sun3dvma.c | 3 +++
For m68k:
Reviewed-by: Geert Uytterhoeven
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*()
Hi Mike,
On Wed, Jan 16, 2019 at 2:46 PM Mike Rapoport wrote:
> Add check for the return value of memblock_alloc*() functions and call
> panic() in case of error.
> The panic message repeats the one used by panicing memblock allocators with
> adjustment of parameters to include only relevant ones.
>
> The replacement was mostly automated with semantic patches like the one
> below with manual massaging of format strings.
>
> @@
> expression ptr, size, align;
> @@
> ptr = memblock_alloc(size, align);
> + if (!ptr)
> + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
In general, you want to use %zu for size_t
> size, align);
>
> Signed-off-by: Mike Rapoport
Thanks for your patch!
> 74 files changed, 415 insertions(+), 29 deletions(-)
I'm wondering if this is really an improvement?
For the normal memory allocator, the trend is to remove printing of errors
from all callers, as the core takes care of that.
> --- a/arch/alpha/kernel/core_marvel.c
> +++ b/arch/alpha/kernel/core_marvel.c
> @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str)
>
> sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port);
> name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES);
> + if (!name)
> + panic("%s: Failed to allocate %lu bytes\n", __func__,
%zu, as strlen() returns size_t.
> + strlen(tmp) + 1);
> strcpy(name, tmp);
>
> return name;
> @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe)
> }
>
> io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES);
> + if (!io7)
> + panic("%s: Failed to allocate %lu bytes\n", __func__,
%zu, as sizeof() returns size_t.
Probably there are more. Yes, it's hard to get them right in all callers.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 13/21] arch: don't memset(0) memory returned by memblock_alloc()
On Wed, Jan 16, 2019 at 2:45 PM Mike Rapoport wrote:
> memblock_alloc() already clears the allocated memory, no point in doing it
> twice.
>
> Signed-off-by: Mike Rapoport
> arch/m68k/mm/mcfmmu.c | 1 -
For m68k part:
Acked-by: Geert Uytterhoeven
> --- a/arch/m68k/mm/mcfmmu.c
> +++ b/arch/m68k/mm/mcfmmu.c
> @@ -44,7 +44,6 @@ void __init paging_init(void)
> int i;
>
> empty_zero_page = (void *) memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> - memset((void *) empty_zero_page, 0, PAGE_SIZE);
>
> pg_dir = swapper_pg_dir;
> memset(swapper_pg_dir, 0, sizeof(swapper_pg_dir));
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH trivial] xen/balloon: Grammar s/Is it/It is/
Signed-off-by: Geert Uytterhoeven --- drivers/xen/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig index 90d387b50ab747f5..7f42d41f66ee98e3 100644 --- a/drivers/xen/Kconfig +++ b/drivers/xen/Kconfig @@ -86,7 +86,7 @@ config XEN_SCRUB_PAGES_DEFAULT help Scrub pages before returning them to the system for reuse by other domains. This makes sure that any confidential data - is not accidentally visible to other domains. Is it more + is not accidentally visible to other domains. It is more secure, but slightly less efficient. This can be controlled with xen_scrub_pages=0 parameter and /sys/devices/system/xen_memory/xen_memory0/scrub_pages. -- 2.17.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 10/13] block: remove bvec_to_phys
On Tue, Sep 4, 2018 at 9:23 PM Christoph Hellwig wrote:
> We only use it in biovec_phys_mergeable and a m68k paravirt driver,
> so just opencode it there. Also remove the pointless unsigned long cast
> for the offset in the opencoded instances.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Geert Uytterhoeven
> ---
> arch/m68k/emu/nfblock.c | 2 +-
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Build regressions/improvements in v4.18-rc4
On Mon, Jul 9, 2018 at 9:37 AM Geert Uytterhoeven wrote:
> JFYI, when comparing v4.18-rc4[1] to v4.18-rc3[3], the summaries are:
> - build errors: +17/-0
+ error: "__sync_icache_dcache" [drivers/xen/xen-privcmd.ko]
undefined!: => N/A
arm64-allmodconfig (__sync_icache_dcache() needs an EXPORT_SYMBOL_GPL?)
+ error:
/opt/cross/kisskb/korg/gcc-8.1.0-nolibc/nds32le-linux/bin/nds32le-linux-ld:
direct-io.c: undefined reference to `__ashldi3': => .text+0x1858)
+ error:
/opt/cross/kisskb/korg/gcc-8.1.0-nolibc/nds32le-linux/bin/nds32le-linux-ld:
mballoc.c: undefined reference to `__ashrdi3': => .text+0x1bc2)
+ error:
/opt/cross/kisskb/korg/gcc-8.1.0-nolibc/nds32le-linux/bin/nds32le-linux-ld:
mmio.c: undefined reference to `__lshrdi3': => .init.text+0x60)
+ error:
/opt/cross/kisskb/korg/gcc-8.1.0-nolibc/nds32le-linux/bin/nds32le-linux-ld:
super.c: undefined reference to `__ashrdi3': => .text+0x6504)
+ error:
/opt/cross/kisskb/korg/gcc-8.1.0-nolibc/nds32le-linux/bin/nds32le-linux-ld:
tcp_ipv4.c: undefined reference to `__ashldi3': => .text+0x25ac)
+ error:
/opt/cross/kisskb/korg/gcc-8.1.0-nolibc/nds32le-linux/bin/nds32le-linux-ld:
timekeeping.c: undefined reference to `__ashldi3': =>
.init.text+0x1ac)
+ error:
/opt/cross/kisskb/korg/gcc-8.1.0-nolibc/nds32le-linux/bin/nds32le-linux-ld:
timekeeping.c: undefined reference to `__lshrdi3': =>
.init.text+0x1f0), .init.text+0x1ec)
+ error: direct-io.c: undefined reference to `__ashldi3': =>
.text+0x16a0), .text+0x16a4), .text+0x185c)
+ error: mballoc.c: undefined reference to `__ashldi3': =>
.text+0x1c46), .text+0x1c70), .text+0x1c4a), .text+0x1c6c)
+ error: mballoc.c: undefined reference to `__ashrdi3': =>
.text+0x1c0e), .text+0x1c12), .text+0x1bbe)
+ error: mmio.c: undefined reference to `__lshrdi3': => .init.text+0x5c)
+ error: super.c: undefined reference to `__ashldi3': =>
.text+0x7fe0), .text+0x7fe4)
+ error: super.c: undefined reference to `__ashrdi3': => .text+0x6500)
+ error: super.c: undefined reference to `__lshrdi3': =>
.text+0x7e38), .text+0x7e34)
+ error: tcp_ipv4.c: undefined reference to `__ashldi3': => .text+0x25a8)
+ error: timekeeping.c: undefined reference to `__ashldi3': =>
.init.text+0x1a8)
nds32le/nds32-defconfig (patch available)
> [1]
> http://kisskb.ellerman.id.au/kisskb/head/1e4b044d22517cae7047c99038abb23243ca/
> (232 out of 244 configs)
> [3]
> http://kisskb.ellerman.id.au/kisskb/head/021c91791a5e7e85c567452f1be3e4c2c6cb6063/
> (231 out of 244 configs)Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
