pt., 21 sty 2022 o 18:52 Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx/
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
>
>
