Re: [Bug Report] Multiple S390x KUNIT clk failures

Audra Mitchell
2024-05-30T20:12:41Z

On Wed, May 29, 2024 at 12:39:34PM -0700, Stephen Boyd wrote:
> Quoting msalter@redhat.com (2024-05-28 15:53:48)
> > On Tue, 2024-05-28 at 16:49 -0400, Audra Mitchell wrote:
> > >
> > > I spent some time last week or so working on debugging these failures and I
> > > believe I have found the problem. I reached out to Malk Salter for advice on
> > > the best way to move forward with a fix on Friday the 17th, but he was on
> > > PTO for the last week. I was waiting for his reply before I replied to this
> > > thread.
> > >
> > > Also as a side note, I also ran into the same issue as Stephen with running
> > > the kunit tests on s390 QEMU. I did not pursue resolving that issue and
> > > instead just compiled the test as a module.
> > >
> > > For clarity, this is what I sent to Mark and were I believe the failure is
> > > occurring:
> > >
> > > The tests create a pretend clk-gate and use a "fake_reg" to emulate
> > > the expected behavior of the clk_gate->reg. I added some debug
> > > statements to the driver and noticed that the reg changes after
> > > initialization to -1. I also noticed that we call this to read the
> > > data in the clk-gate->reg:
> > >
> > > static inline u32 clk_gate_readl(struct clk_gate *gate)
> > > {
> > >         if (gate->flags & CLK_GATE_BIG_ENDIAN)
> > >                 return ioread32be(gate->reg);
> > >
> > >         return readl(gate->reg);
> > > }
> > >
> > > However, it does not look like ioread32be is defined for s390, so
> >
> > It is defined. arch/s390/include/asm/io.h defines:
> >
> > #define __raw_readl zpci_read_u32
> >
> > and then includes include/asm-generic/io.h which has:
> >
> > static inline u32 readl(const volatile void __iomem *addr)
> > {
> > u32 val;
> >
> > log_read_mmio(32, addr, _THIS_IP_, _RET_IP_);
> > __io_br();
> > val = __le32_to_cpu((__le32 __force)__raw_readl(addr));
> > __io_ar(val);
> > log_post_read_mmio(val, 32, addr, _THIS_IP_, _RET_IP_);
> > return val;
> > }
> > ...
> > static inline u32 ioread32be(const volatile void __iomem *addr)
> > {
> > return swab32(readl(addr));
> > }
> >
> > which should do the right thing (s390 being BE and readl() is for 32-bit LE reads).
> >
> > But I don't know the s390 compiler or ISA, so I'm not sure where the zpci_load
> > is coming from.
> >
>
> So the problem is that the zpci_read_u32() fails and returns -1?
>
> This test isn't the best because it uses fakes iomem and architectures
> may not like that. We really need to implement something in KUnit core
> to allocate a fake iomem region and then plumb that through all the
> architectures so that the iomem functions like readl, writel, etc. go a
> different direction when the pointer is for the fake region.
>
> Probably the best thing to do in the short term here is to prevent this
> test from running on S390 via Kconfig.

Hey Stephen, thanks a bunch for looking at this. I do not have a lot of
experience with s390 to vote one way or another for how best to approach
resolving this problem.

By the way zpci_load is coming from here:

readl calls __raw_readl
s390 defines raw_readl as zpci_read_u32:

#define __raw_readl zpci_read_u32

And zpci_read is defined here, which then calls zpci_load:

#define zpci_read(LENGTH, RETTYPE) \
static inline RETTYPE zpci_read_##RETTYPE(const volatile void __iomem *addr) \
{ \
u64 data; \
int rc; \
\
rc = zpci_load(&data, addr, LENGTH); \
if (rc) \
data = -1ULL; \
return (RETTYPE) data; \
}

Thanks again!