lkml.org 
[lkml]   [2020]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 3/3] bus: mhi: core: Remove link_status() callback
    From
    Date
    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@codeaurora.org>
    >
    > 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?

    >
    > For this,
    >
    > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
    >
    > 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.

    \
     
     \ /
      Last update: 2020-04-07 17:04    [W:3.466 / U:0.196 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site