Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk

[Ulf Hansson]

[...]

> >>
> >> Agree. Seems I need to create an alternate path to forcefully gain
> >> access to the host for the case of panic write. As you pointed out
> >> mmc_claim_host(), mmc_release_host() and runtime PM can create issues.
> >>
> >> >The question is, can/should we rely on mmc_claim_host() to succeed in
> >> >this path? Maybe it will work, in cases when there is no ongoing
> >> >request, as it means the host should be available to be immediately
> >> >claimed. Although, then the problem ends up with runtime PM, as if
> >> >the host is available for claiming, it's likely that the host is runtime
> >suspended...
> >> >
> >>
> >> An extra check can be added to see if host was runtime suspended ahead
> >> of panic write attempt.
> >
> >What if that is the case, should we just return an error?
> >
> Yes.
>
> >Moreover, even the device belonging to the mmc card can be runtime
> >suspended too. So if that is the case, we should return an error too?
> >
>
> Yes, same here.
>
> Assuming ->req_cleanup_pending() properly terminates the ongoing DMA transfers,
> mmc_claim_host() and mmc_release_host() can be dropped in panic write case
> as it has then exclusive access from then on.

Again, I think you have misunderstood how it works from the core point of view.

->req_cleanup_pending() will never be able to terminate a request in a
proper way, without involving the mmc core's knowledge about the
eMMC/SD protocol.

Yes, it could clean up from an ongoing DMA transfer, but that doesn't
bring the eMMC/SD card back into a state where it can accept new
requests.

>
>
> >[...]
> >
> >> >> >[...]
> >> >> >
> >> >> >Having said the above, I am not entirely convinced that it makes
> >> >> >sense to support this, at all.
> >> >> >
> >> >> >Not only, will the support be highly fragile from the mmc core
> >> >> >point of view, but there is also a significant complexity for an
> >> >> >mmc host driver to support this (at least in general).
> >> >> >
> >> >>
> >> >> I am not sure if the comments on host driver complexity is true.
> >> >> Terminating ongoing requests and introducing polling functions on
> >> >> host drivers should be straight forward. None those would disturb
> >> >> the core functionality. They are completely independent.
> >> >
> >> >I think you are underestimating the part with terminating ongoing
> >> >requests. It sounds to me that you really haven't been terminating
> >> >any requests at all, but rather just doing a reset of the mmc
> >> >controller (which is what I observed in patch2).
> >> >
> >>
> >> No, it's not true. I am not doing any reset. Please point me to
> >> specific code snippet where you have observed this.
> >
> >I was looking at patch2 and the ->req_cleanup_pending() callback that you
> >have assigned to cvm_req_cleanup_pending().
> >
> >In there you clear a potentially running DMA job, which is *kind* of a reset of
> >the controller. More importantly, it's definitely *not* terminating an ongoing
> >request, in a way that you can expect the eMMC/SD card to be ready for new
> >communications afterwards. This is my main point.
> >
>
> I am not sure that clearing an ongoing DMA will reset the controller. These are host
> controller specific. The idea is: To drop ongoing transfers, whatever a host software
> has to do it will does through this cleanup function. We may not generalize this,
> providing a hook and letting each host controller handling it seems better.

I fully understand the approach, but again, it doesn't work. Sorry.

As I said, the only thing that *could* work is to call
mmc_claim_host() in the panic write hook -  and hope for the best. If
it succeeds, there is nothing to cleanup or terminate.

Kind regards
Uffe