Message-ID: <CAOtMz3NE4z-=eZyE_xzs68xDpf47-bqv+fCE=ATUx+4ba_p6zg@mail.gmail.com>
Date:   Fri, 28 Jan 2022 15:46:19 +0100
From:   Jan Dąbroś <jsd@...ihalf.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Wolfram Sang <wsa@...nel.org>,
        Raul E Rangel <rrangel@...omium.org>,
        Marcin Wojtas <mw@...ihalf.com>,
        Grzegorz Jaszczyk <jaz@...ihalf.com>, upstream@...ihalf.com
Subject: Re: [PATCH 2/2] i2c: designware: Add AMD PSP I2C bus support

pt., 21 sty 2022 o 18:52 Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> napisał(a):
>
> On Fri, Jan 21, 2022 at 11:20:19AM +0100, Jan Dąbroś wrote:
> > czw., 20 sty 2022 o 15:21 Andy Shevchenko
> > <andriy.shevchenko@...ux.intel.com> napisał(a):
> > > On Thu, Jan 20, 2022 at 01:16:21AM +0100, Jan Dabros wrote:
>
> ...
>
> > > > Add new entry in MAINTAINERS file to cover new module.
> > >
> > > It's confusing. You added yourself as a reviewer for I2C DesignWare driver,
> > > which is great, but not described in the commit message.
> >
> > Should I rephrase this sentence (to be more specific that I may be
> > helpful for reviewing amdpsp.c module) or rather you mean to exclude
> > drivers/i2c/busses/i2c-designware-amdpsp.c from the rest of
> > designware-* modules and create separate entry?
> >
> > Actually initially I wasn't planning to modify MAINTAINERS (after all
> > I'm not an I2C DesignWare expert) until run checkpatch.pl which
> > recommended to do so when adding new file. Eventually for me it made
> > some sense since I have a platform equipped with AMD Cezanne SoC and I
> > will be able to review and test potential changes in
> > i2c-designware-amdpsp.c or in general around semaphore areas.
> >
> > This may also work with different model, similar to how you pointed me
> > to Hans as an owner of Bay Trail platform who is acquinted with how
> > its i2c semaphore is working. I will simply remove myself from the
> > MAINTAINERS file and you can add me to the threads if there is
> > anything requiring my help.
> >
> > Let me know which way is working for you. I just thought it is not
> > good to leave you alone with a new module which you cannot actually
> > test and don't have deep insight about how PSP-x86 communications
> > works.
>
> You have a few options:
> - leave it for us (but it probably won't go well in long-term as you noticed)
> - add yourself as a reviewer (it doesn't require to review everything, but
>   you will get all i2c DesignWare driver changes)

I think this is the best option. So I will leave initial change to the
MAINTAINERS as is, but will put few more words of description into
commit message.

> - add a new MAINTAINERS database entry where you can put yourself for that
>   file even as a maintainaer
>
> ...
>
> > > > +#include <asm/msr.h>
> > >
> > > Usually linux/* followed by asm/*.
> > >
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/psp-sev.h>
> > >
> > > types.h?
> >
> > I need to take a deeper look at the headers included here, especially
> > considering errors pointed by kernel test robot. Not sure why I
> > haven't seen any issues on my setup.
>
> The problem here is not so visible. Headers should be a compromise between
> what is really used and what we may include for that. There are headers
> that guaranteed to be included by others, otherwise it will be an implicit
> dependency which is not good in cases of generic headers, such as types.h.

ACK. I included couple of new headers in v2.

>
> ...
>
> > > > +union psp_req_buffer_hdr {
> > > > +     struct {
> > > > +             u32 total_size;
> > > > +             u32 status;
> > > > +     } __packed;
> > >
> > > What does packet bring you here?
> >
> > In this particular case binary-wise nothing, I can as well drop this.
> > It may be necessary if there are some changes in this structs fields
> > in future (e.g changing total_size to u8), since PSP expects every
> > field to be byte-aligned.
>
> _packed will make another thing which you probably won't need, it brings
> the entire structure to be possible on unaligned addresses and then some
> warnings from compiler may be issued even if there is no problem in the
> flow.
>
> Read this discussion: https://lore.kernel.org/linux-media/20220110224656.266536-1-sakari.ailus@linux.intel.com/

OK, I see your point - thanks for this pointer, something new for me.

>
> > > > +     u64 hdr_val;
> > >
> > > And why does this not have the same alignment since it's also part of
> > > the union?
> >
> > __packed is not about alignment of the whole struct/union
>
> It's. See above.

ACK.

>
> > but about
> > lack of padding between its fields. As above - in this particular case
> > with two u32 it doesn't matter.
>
> ...
>
> > > > +struct psp_i2c_req {
> > > > +     union psp_req_buffer_hdr hdr;
> > > > +     enum psp_i2c_req_type type;
> > >
> > > > +} __packed __aligned(32);
> > >
> > > Can you explain, what this means and how it's supposed to work?
> >
> > This means that each instance of the struct should be aligned (32)
> > while at the same time no padding within members - thus this may
> > result in non-aligned addresses of members.
>
> 32 bytes? And on unaligned address at the same time.

Yes, it was initial recommendation. I left this as is in v2, will work
with AMD to see if we can lower (get rid of) this requirement.

>
> ...
>
> > > > +union psp_mbox_cmd_reg {
> > > > +     struct psp_mbox_cmd_fields {
> > > > +             u16 mbox_status;
> > > > +             u8 mbox_cmd;
> > > > +             u8 reserved:6;
> > > > +             u8 recovery:1;
> > > > +             u8 ready:1;
> > >
> > > > +     } __packed fields;
> > >
> > > So, what is the __packed purpose here?
> >
> > As in all above cases - considering current layout of members and
> > their sizes dropping `__packed` will not results in any errors.
> >
> > However PSP expects all members os structs within shared buffers to be
> > byte-aligned, that's why I've added this attributes to be on the safe
> > side. If you think this doesn't make sense, I can remove them - in
> > (very unlikely) case of changes, one will need to add this specifier.
>
> I guess you don't need them at all in this case in any of the data structure
> you created here.

ACK, I removed unnecessary __packed attributes in v2.

> ...
>
> > > > +     struct psp_mbox *mbox = (struct psp_mbox *)mbox_iomem;
> > >
> > > Heck, no!
> >
> > I need to get acquinted to the kernel-reviewer language:) Pardon my
> > ignorance, but just to be sure I get what you mean here:
> > I'm using global mbox_iomem to keep address of PSP mailbox in IO
> > memory. Casting this to struct psp_mbox layout here, to make access
> > more convenient.
> > Your point here is that:
> > 1. I should move the assignment out from the variable declaration part
> > of this function;
> > 2. I should use ioremap/iounmap each time in psp_send_cmd instead of
> > using it once in `probe` and unmap in `remove`?
> > I thought about this option as to be less effective performance-wise
> > (even though I can get rid of one global variable).
> > 3. Something else?
>
> Casting an __iomem pointer to the some struct without keeping it. I believe
> sparse should blow because of this.

Oh right.. pretty obvious. Fixed.

>
> ...
>
> > > > +     /* Fill address of command-response buffer */
> > > > +     writeq((uintptr_t)__psp_pa((void *)req), &mbox->i2c_req_addr);
> > >
> > > What does this voodoo mean?!
> >
> > Basically I need to take physical address (__psp_pa) of request buffer
> > req and write this down into mailbox IO memory.
> > This should be spread into more lines with some comments, is this your point?
>
> It needs much better comment explaining what is this address and its meaning
> for the hardware and why you need physical address here (DMA?). For me it looks
> like a voodoo. Ah, and not using phys_addr_t / dma_addr_t / etc type here, but
> uintptr_t just adds a confusion.

ACK.

> ...
>
> > > > +     start = jiffies;
> > > > +     do {
> > > > +             if (psp_send_cmd(req)) {
> > > > +                     ret = -EIO;
> > > > +                     goto cleanup;
> > > > +             }
> > > > +
> > > > +             status = check_i2c_req_sts(req);
> > > > +             if (!status) {
> > > > +                     dev_dbg(psp_i2c_dev, "Request accepted by PSP after %ums\n",
> > > > +                             jiffies_to_msecs(jiffies - start));
> > > > +                     ret = 0;
> > > > +                     goto cleanup;
> > > > +             } else if (status == -EBUSY) {
> > > > +                     retry_cnt--;
> > > > +             } else {
> > > > +                     ret = -EIO;
> > > > +                     goto cleanup;
> > > > +             };
> > > > +
> > > > +             /* IF EBUSY, give PSP time to finish its i2c activities */
> > > > +             mdelay(PSP_I2C_REQ_RETRY_DELAY_MSEC);
> > > > +     } while (retry_cnt);
> > >
> > > NIH iopoll.h API(s).
> >
> > I don't think macros avaialble in iopoll.h are suitable here.
> > Procedure above is not about simply reading some IO and waiting for
> > particular condition to be met with this particular value. Eventually
> > `psp_send_cmd()` invokes `psp_wait_cmd()` where I'm using
> > `readl_poll_timeout()`, so on lower level I'm making use of this API.
> > However here I don't see any obvious method how to incorporate
> > iopoll.h API to reach my goal.
>
> You do not go to clean up if and only if status == -EBUSY, so here we have
> a condition. the rest can be moved to a function that you wrap by
> read_poll_timeout_atomic() (pay attention to the macro name).
> Here is the question, btw, why _atomic()? I.o.w. why mdelay() and not msleep()?

Right, this was another error - above code shouldn't be executed in
atomic context. I used read_poll_timeout() in v2.

> > > > +     ret = -ETIMEDOUT;
>
> ...
>
> > > Handle errors first.
>
> > Addressing above two comments - what do you think about below:
> > if (status) {
> >       if (status == -ETIMEDOUT)
> >                dev_err(psp_i2c_dev, "Timed out waiting for PSP to
> > release I2C bus\n");
> >       else
> >                dev_err(psp_i2c_dev, "PSP communication error\n");
>
> >        dev_err(psp_i2c_dev, "PSP communication error\n");
>
> This dup message is not needed, otherwise fine to me.

ACK.

>
> >        psp_i2c_mbox_fail = true;
> >        goto cleanup;
> > }
> >
> > psp_i2c_sem_acquired = jiffies;
> > psp_i2c_access_count++;
> > (...)
>
> ...
>
> > > > +static int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
> > > > +{
> > > > +     int ret;
> > > > +     int i;
> > > > +
> > > > +     dev->semaphore_idx = -1;
> > > > +
> > > > +     for (i = 0; i < ARRAY_SIZE(i2c_dw_semaphore_cb_table); i++) {
> > >
> > > > +             if (!i2c_dw_semaphore_cb_table[i].probe)
> > > > +                     continue;
> > >
> > > Huh?
> >
> > Just to be sure I get your point.
> > Once I found terminating entry, I will get out of the loop and return
> > 0 as there are no semaphores to be "applied". Actually I should
> > probably use `break;` here as there shouldn't be a case when certain
> > type of semaphore installs without `probe` being implemented.
>
> Yes, that's what I though, and rather using ARRAY_SIZE(), use a terminator you
> provided.
>
> Originally you have used two approaches which seems competing with each other:
> - ARRAY_SIZE
> - terminator entry
>
> And on top of that so confusing continue on top of not-found ->probe() that
> makes me think what the meaning of the entry that has set ->remove(), but no
> ->probe().
>
> That said, I would rather see something like
>
>         struct ... *p = &...;
>
>         while (p->probe) {
>                 ...
>                 p++;
>         }

ACK.

> --
> With Best Regards,
> Andy Shevchenko
>
>

Powered by blists - more mailing lists