Re: [Bug Report] Multiple S390x KUNIT clk failures

[Audra Mitchell <audra@redhat.com>]

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!