On Wed, 1 Jun 2022 at 09:41, Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Jun 1, 2022 at 7:52 AM WANG Xuerui <[email protected]> wrote:
> > On 6/1/22 00:01, Huacai Chen wrote:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/chenhuacai/linux-loongson.git/log/?h=loongarch-next
> > > has been updated. Now this branch droped irqchip drivers and pci
> > > drivers. But the existing irqchip drivers need some small adjustment
> > > to avoid build errors [1], and I hope Marc can give an Acked-by.
> > > Thanks.
> > >
> > > This branch can be built with defconfig and allmodconfig (except
> > > drivers/platform/surface/aggregator/controller.c, because it requires
> > > 8bit/16bit cmpxchg, which I was told to remove their support).
> > >
> > > [1] https://lore.kernel.org/lkml/[email protected]/T/#t
> >
> > I see the loongarch-next HEAD has been updated and it's now purely arch
> > changes aside from the two trivial irqchip cleanups. Some other changes
> > to the v11 patchset [1] are included, but arguably minor enough to not
> > invalidate previous Reviewed-by tags.
>
> Very nice! I don't see exactly how the previous build bugs were addressed,
> but I can confirm that this version builds. Regarding the two irqchip patches,
> 621e7015b529 ("irqchip/loongson-liointc: Fix build error for LoongArch") is
> a good way to work around the mips oddity, and I have no problem taking
> that through the asm-generic tree. The other one, f54b4a166023 ("irqchip:
> Adjust Kconfig for Loongson"), looks mostly unnecessary, and I think only
> the LOONGSON_HTPIC change should be included here, while I would
> leave out the COMPILE_TEST changes and instead have the driver
> changes take care of making it possible to keep building it on x86, possibly
> doing
>
> depends on MACH_LOONGSON64 || (COMPILE_TEST && ACPI)
>
> in the future, after the loongarch64 ACPI support is merged.
>
> > After some small tweaks:
> >
> > - adding "#include <asm/irqflags.h>" to arch/loongarch/include/asm/ptrace.h,
> > - adding an arch/loongarch/include/uapi/asm/bpf_perf_event.h with the
> > same content as arch/arm64's, and
> > - adding "depends on ARM64 || X86" to
> > drivers/platform/surface/aggregator/Kconfig,
> >
> > the current loongarch-next HEAD (commit
> > 36552a24f70d21b7d63d9ef490561dbdc13798d7) now passes allmodconfig build
> > (with CONFIG_WERROR disabled; my Gentoo-flavored gcc-12 seems to emit
> > warnings on a few drivers).
>
> The only one of these issues that I see is the surface aggregator one.
> I think we can address all three as follow-up fixes after -rc1 if the port
> gets merged and these are still required.
>
> > The majority of userspace ABI has been stable for a few months already,
> > after the addition of orig_a0 and removal of newfstatat; the necessary
> > changes to switch to statx are already reviewed [2] / merged [3], and
> > have been integrated into the LoongArch port of Gentoo for a while. Eric
> > looked at the v11 and gave comments, and changes were made according to
> > the suggestions, but it'd probably better to get a proper Reviewed-by.
>
> Right.
>
> > Among the rest of patches, I think maybe the EFI/boot protocol part
> > still need approval/ack from the EFI maintainer. However because the
> > current port isn't going to be able to run on any real hardware, maybe
> > that part could be done later; I'm not sure if the unacknowledged EFI
> > bits should be removed as well.
>
> Ard, do you have any last comments on this?
>
It would be nice if the questions I raised against the previous
revision (v11) were addressed (or at least answered) first. In
general, I think this is feeling a bit rushed and IMHO we should
probably defer this to the next cycle.
Hi Ard,
On 6/2/22 00:01, Ard Biesheuvel wrote:
> On Wed, 1 Jun 2022 at 09:41, Arnd Bergmann <[email protected]> wrote:
>> On Wed, Jun 1, 2022 at 7:52 AM WANG Xuerui <[email protected]> wrote:
>>> On 6/1/22 00:01, Huacai Chen wrote:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/chenhuacai/linux-loongson.git/log/?h=loongarch-next
>>>> has been updated. Now this branch droped irqchip drivers and pci
>>>> drivers. But the existing irqchip drivers need some small adjustment
>>>> to avoid build errors [1], and I hope Marc can give an Acked-by.
>>>> Thanks.
>>>>
>>>> This branch can be built with defconfig and allmodconfig (except
>>>> drivers/platform/surface/aggregator/controller.c, because it requires
>>>> 8bit/16bit cmpxchg, which I was told to remove their support).
>>>>
>>>> [1] https://lore.kernel.org/lkml/[email protected]/T/#t
>>> I see the loongarch-next HEAD has been updated and it's now purely arch
>>> changes aside from the two trivial irqchip cleanups. Some other changes
>>> to the v11 patchset [1] are included, but arguably minor enough to not
>>> invalidate previous Reviewed-by tags.
>> Very nice! I don't see exactly how the previous build bugs were addressed,
>> but I can confirm that this version builds. Regarding the two irqchip patches,
>> 621e7015b529 ("irqchip/loongson-liointc: Fix build error for LoongArch") is
>> a good way to work around the mips oddity, and I have no problem taking
>> that through the asm-generic tree. The other one, f54b4a166023 ("irqchip:
>> Adjust Kconfig for Loongson"), looks mostly unnecessary, and I think only
>> the LOONGSON_HTPIC change should be included here, while I would
>> leave out the COMPILE_TEST changes and instead have the driver
>> changes take care of making it possible to keep building it on x86, possibly
>> doing
>>
>> depends on MACH_LOONGSON64 || (COMPILE_TEST && ACPI)
>>
>> in the future, after the loongarch64 ACPI support is merged.
>>
>>> After some small tweaks:
>>>
>>> - adding "#include <asm/irqflags.h>" to arch/loongarch/include/asm/ptrace.h,
>>> - adding an arch/loongarch/include/uapi/asm/bpf_perf_event.h with the
>>> same content as arch/arm64's, and
>>> - adding "depends on ARM64 || X86" to
>>> drivers/platform/surface/aggregator/Kconfig,
>>>
>>> the current loongarch-next HEAD (commit
>>> 36552a24f70d21b7d63d9ef490561dbdc13798d7) now passes allmodconfig build
>>> (with CONFIG_WERROR disabled; my Gentoo-flavored gcc-12 seems to emit
>>> warnings on a few drivers).
>> The only one of these issues that I see is the surface aggregator one.
>> I think we can address all three as follow-up fixes after -rc1 if the port
>> gets merged and these are still required.
>>
>>> The majority of userspace ABI has been stable for a few months already,
>>> after the addition of orig_a0 and removal of newfstatat; the necessary
>>> changes to switch to statx are already reviewed [2] / merged [3], and
>>> have been integrated into the LoongArch port of Gentoo for a while. Eric
>>> looked at the v11 and gave comments, and changes were made according to
>>> the suggestions, but it'd probably better to get a proper Reviewed-by.
>> Right.
>>
>>> Among the rest of patches, I think maybe the EFI/boot protocol part
>>> still need approval/ack from the EFI maintainer. However because the
>>> current port isn't going to be able to run on any real hardware, maybe
>>> that part could be done later; I'm not sure if the unacknowledged EFI
>>> bits should be removed as well.
>> Ard, do you have any last comments on this?
>>
> It would be nice if the questions I raised against the previous
> revision (v11) were addressed (or at least answered) first. In
> general, I think this is feeling a bit rushed and IMHO we should
> probably defer this to the next cycle.
Actually I think Huacai did reply to your review on v11:
https://lore.kernel.org/all/CAAhV-H7KAg8RxN7M=WiOOh0fDhEKTyqrwp6V-SC0cyR0iMrdeg@mail.gmail.com/.
It's a bit unfortunate that he probably didn't justify some of the
approaches enough, and it's especially unfortunate that some of the
points (like maybe the kernel version string in the EFI stub header) are
result of their internal discussion, which I presume to be especially
hard to change due to their particularly worrying corporate dynamics...
But again, my point is that the userspace ABI in particular is *not*
rushed -- it has been brewing since v1 of the port which is already
several months ago, and multiple distro-building efforts are already
underway. We (LoongArch distro packagers) want to freeze the userspace
ABI so that many downstream efforts wouldn't be blocked by the merging
of kernel port.
As the boot protocol is technically not part of the userspace ABI that
toolchains care about, and we already know it'll be a rather
standards-compliant UEFI implementation even if this part gets dropped
for brewing one more cycle, would taking this part out work for you?
Hi, Ard,
On Thu, Jun 2, 2022 at 12:44 AM WANG Xuerui <[email protected]> wrote:
>
> Hi Ard,
>
> On 6/2/22 00:01, Ard Biesheuvel wrote:
> > On Wed, 1 Jun 2022 at 09:41, Arnd Bergmann <[email protected]> wrote:
> >> On Wed, Jun 1, 2022 at 7:52 AM WANG Xuerui <[email protected]> wrote:
> >>> On 6/1/22 00:01, Huacai Chen wrote:
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/chenhuacai/linux-loongson.git/log/?h=loongarch-next
> >>>> has been updated. Now this branch droped irqchip drivers and pci
> >>>> drivers. But the existing irqchip drivers need some small adjustment
> >>>> to avoid build errors [1], and I hope Marc can give an Acked-by.
> >>>> Thanks.
> >>>>
> >>>> This branch can be built with defconfig and allmodconfig (except
> >>>> drivers/platform/surface/aggregator/controller.c, because it requires
> >>>> 8bit/16bit cmpxchg, which I was told to remove their support).
> >>>>
> >>>> [1] https://lore.kernel.org/lkml/[email protected]/T/#t
> >>> I see the loongarch-next HEAD has been updated and it's now purely arch
> >>> changes aside from the two trivial irqchip cleanups. Some other changes
> >>> to the v11 patchset [1] are included, but arguably minor enough to not
> >>> invalidate previous Reviewed-by tags.
> >> Very nice! I don't see exactly how the previous build bugs were addressed,
> >> but I can confirm that this version builds. Regarding the two irqchip patches,
> >> 621e7015b529 ("irqchip/loongson-liointc: Fix build error for LoongArch") is
> >> a good way to work around the mips oddity, and I have no problem taking
> >> that through the asm-generic tree. The other one, f54b4a166023 ("irqchip:
> >> Adjust Kconfig for Loongson"), looks mostly unnecessary, and I think only
> >> the LOONGSON_HTPIC change should be included here, while I would
> >> leave out the COMPILE_TEST changes and instead have the driver
> >> changes take care of making it possible to keep building it on x86, possibly
> >> doing
> >>
> >> depends on MACH_LOONGSON64 || (COMPILE_TEST && ACPI)
> >>
> >> in the future, after the loongarch64 ACPI support is merged.
> >>
> >>> After some small tweaks:
> >>>
> >>> - adding "#include <asm/irqflags.h>" to arch/loongarch/include/asm/ptrace.h,
> >>> - adding an arch/loongarch/include/uapi/asm/bpf_perf_event.h with the
> >>> same content as arch/arm64's, and
> >>> - adding "depends on ARM64 || X86" to
> >>> drivers/platform/surface/aggregator/Kconfig,
> >>>
> >>> the current loongarch-next HEAD (commit
> >>> 36552a24f70d21b7d63d9ef490561dbdc13798d7) now passes allmodconfig build
> >>> (with CONFIG_WERROR disabled; my Gentoo-flavored gcc-12 seems to emit
> >>> warnings on a few drivers).
> >> The only one of these issues that I see is the surface aggregator one.
> >> I think we can address all three as follow-up fixes after -rc1 if the port
> >> gets merged and these are still required.
> >>
> >>> The majority of userspace ABI has been stable for a few months already,
> >>> after the addition of orig_a0 and removal of newfstatat; the necessary
> >>> changes to switch to statx are already reviewed [2] / merged [3], and
> >>> have been integrated into the LoongArch port of Gentoo for a while. Eric
> >>> looked at the v11 and gave comments, and changes were made according to
> >>> the suggestions, but it'd probably better to get a proper Reviewed-by.
> >> Right.
> >>
> >>> Among the rest of patches, I think maybe the EFI/boot protocol part
> >>> still need approval/ack from the EFI maintainer. However because the
> >>> current port isn't going to be able to run on any real hardware, maybe
> >>> that part could be done later; I'm not sure if the unacknowledged EFI
> >>> bits should be removed as well.
> >> Ard, do you have any last comments on this?
> >>
> > It would be nice if the questions I raised against the previous
> > revision (v11) were addressed (or at least answered) first. In
> > general, I think this is feeling a bit rushed and IMHO we should
> > probably defer this to the next cycle.
>
> Actually I think Huacai did reply to your review on v11:
> https://lore.kernel.org/all/CAAhV-H7KAg8RxN7M=WiOOh0fDhEKTyqrwp6V-SC0cyR0iMrdeg@mail.gmail.com/.
> It's a bit unfortunate that he probably didn't justify some of the
> approaches enough, and it's especially unfortunate that some of the
> points (like maybe the kernel version string in the EFI stub header) are
> result of their internal discussion, which I presume to be especially
> hard to change due to their particularly worrying corporate dynamics...
I'm sorry that you haven't seen my reply, but as Xuerui said, I have
replied to your review. :)
Since you didn't reply to my answers again, I supposed that you
consider "everything is OK". :)
Now I plan to send V13, with the following changes:
1, Remove kernel_version string in efistub;
2, Remove the boardinfo knob in /sys/firmware/efi;
3, Add a reference in the commit message to explain while we need a
magic number [1].
[1] https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00215.html
Huacai
>
> But again, my point is that the userspace ABI in particular is *not*
> rushed -- it has been brewing since v1 of the port which is already
> several months ago, and multiple distro-building efforts are already
> underway. We (LoongArch distro packagers) want to freeze the userspace
> ABI so that many downstream efforts wouldn't be blocked by the merging
> of kernel port.
>
> As the boot protocol is technically not part of the userspace ABI that
> toolchains care about, and we already know it'll be a rather
> standards-compliant UEFI implementation even if this part gets dropped
> for brewing one more cycle, would taking this part out work for you?
>