On Tue, Apr 07, 2020 at 09:03:28AM -0600, Jeffrey Hugo wrote:
> On 4/6/2020 11:58 PM, Manivannan Sadhasivam wrote:
> > Hi Jeff,
> >
> > On Mon, Apr 06, 2020 at 03:04:37PM -0600, Jeffrey Hugo wrote:
> > > If the MHI core detects invalid data due to a PCI read, it calls into
> > > the controller via link_status() to double check that the link is infact
> > > down. All in all, this is pretty pointless, and racy. There are no good
> > > reasons for this, and only drawbacks.
> > >
> > > Its pointless because chances are, the controller is going to do the same
> > > thing to determine if the link is down - attempt a PCI access and compare
> > > the result. This does not make the link status decision any smarter.
> > >
> > > Its racy because its possible that the link was down at the time of the
> > > MHI core access, but then recovered before the controller access. In this
> > > case, the controller will indicate the link is not down, and the MHI core
> > > will precede to use a bad value as the MHI core does not attempt to retry
> > > the access.
> > >
> > > Retrying the access in the MHI core is a bad idea because again, it is
> > > racy - what if the link is down again? Furthermore, there may be some
> > > higher level state associated with the link status, that is now invalid
> > > because the link went down.
> > >
> > > The only reason why the MHI core could see "invalid" data when doing a PCI
> > > access, that is actually valid, is if the register actually contained the
> > > PCI spec defined sentinel for an invalid access. In this case, it is
> > > arguable that the MHI implementation broken, and should be fixed, not
> > > worked around.
> > >
> > > Therefore, remove the link_status() callback before anyone attempts to
> > > implement it.
> > >
> > > Signed-off-by: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx>
> >
> > LGTM. But as per the IRC discussion I'd like the mhi_reg_read() to be
> > implemented as a callback in mhi_controller struct inorder to truly make MHI
> > a PCI agnostic bus.
> >
> > Since we don't have any controller driver in mainline, I think it is the
> > good time to do this change.
>
> No problem. I thought you might prefer that approach, hence the discussion.
> :)
>
> Do you want that included in this change, or as a follow up?
>
You can add a patch in this series itself.
Thanks,
Mani
> >
> > For this,
> >
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> >
> > Thanks,
> > Mani
> >
> > > ---
> > > drivers/bus/mhi/core/init.c | 6 ++----
> > > drivers/bus/mhi/core/main.c | 5 ++---
> > > include/linux/mhi.h | 2 --
> > > 3 files changed, 4 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > > index b38359c..2af08d57 100644
> > > --- a/drivers/bus/mhi/core/init.c
> > > +++ b/drivers/bus/mhi/core/init.c
> > > @@ -812,10 +812,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> > > if (!mhi_cntrl)
> > > return -EINVAL;
> > > - if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put)
> > > - return -EINVAL;
> > > -
> > > - if (!mhi_cntrl->status_cb || !mhi_cntrl->link_status)
> > > + if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put ||
> > > + !mhi_cntrl->status_cb)
> > > return -EINVAL;
> > > ret = parse_config(mhi_cntrl, config);
> > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > index eb4256b..473278b8 100644
> > > --- a/drivers/bus/mhi/core/main.c
> > > +++ b/drivers/bus/mhi/core/main.c
> > > @@ -20,9 +20,8 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
> > > {
> > > u32 tmp = readl(base + offset);
> > > - /* If there is any unexpected value, query the link status */
> > > - if (PCI_INVALID_READ(tmp) &&
> > > - mhi_cntrl->link_status(mhi_cntrl))
> > > + /* If the value is invalid, the link is down */
> > > + if (PCI_INVALID_READ(tmp))
> > > return -EIO;
> > > *out = tmp;
> > > diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> > > index ad19960..be704a4 100644
> > > --- a/include/linux/mhi.h
> > > +++ b/include/linux/mhi.h
> > > @@ -335,7 +335,6 @@ struct mhi_controller_config {
> > > * @syserr_worker: System error worker
> > > * @state_event: State change event
> > > * @status_cb: CB function to notify power states of the device (required)
> > > - * @link_status: CB function to query link status of the device (required)
> > > * @wake_get: CB function to assert device wake (optional)
> > > * @wake_put: CB function to de-assert device wake (optional)
> > > * @wake_toggle: CB function to assert and de-assert device wake (optional)
> > > @@ -417,7 +416,6 @@ struct mhi_controller {
> > > void (*status_cb)(struct mhi_controller *mhi_cntrl,
> > > enum mhi_callback cb);
> > > - int (*link_status)(struct mhi_controller *mhi_cntrl);
> > > void (*wake_get)(struct mhi_controller *mhi_cntrl, bool override);
> > > void (*wake_put)(struct mhi_controller *mhi_cntrl, bool override);
> > > void (*wake_toggle)(struct mhi_controller *mhi_cntrl);
> > > --
> > > Qualcomm Technologies, Inc. is a member of the
> > > Code Aurora Forum, a Linux Foundation Collaborative Project.
>
>
> --
> Jeffrey Hugo
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
