Re: [PATCH v2 6/6] powerpc/pseries: Add papr-platform-dump character driver for dump retrieval

["Michal Suchánek" <msuchanek@suse.de>]

On Thu, Feb 06, 2025 at 10:34:42AM -0800, Haren Myneni wrote:
> On Thu, 2025-02-06 at 16:32 +0100, Michal Suchánek wrote:
> > On Thu, Feb 06, 2025 at 07:28:14AM -0800, Haren Myneni wrote:
> > > On Thu, 2025-02-06 at 10:18 +0100, Michal Suchánek wrote:
> > > > On Wed, Feb 05, 2025 at 11:51:19PM -0800, Haren Myneni wrote:
> > > > > On Wed, 2025-02-05 at 15:28 +0100, Michal Suchánek wrote:

> > > > > > > +
> > > > > > > +	if (params->status != RTAS_IBM_PLATFORM_DUMP_COMPLETE)
> > > > > > > +		pr_warn("Platform dump is not complete, but
> > > > > > > requested "
> > > > > > > +			"to invalidate dump for ID(%llu)\n",
> > > > > > > +			dump_tag);
> > > > > > 
> > > > > > Not sure if something should be done here or if relying on
> > > > > > translation
> > > > > > of the error from the RTAS call is advisable.
> > > > > 
> > > > > This check just diplays message in case if the user initiated
> > > > > to
> > > > > invalidate the dump without saving it completely. Then
> > > > > invalidates
> > > > > the
> > > > > dump with RTAS call and retuns the RTAS return value. 
> > > > > 
> > > > > As mentioned above, platform-dump is available only on non-HMC
> > > > > based
> > > > > systems. So invoke the collection of dump by BMC based
> > > > > interface,
> > > > > not
> > > > > widely used. I can remove this check if preferred.
> > > > 
> > > > From the previous discussion it sounds like trying to invalidate
> > > > the
> > > > dump without first reading it in full is an error.
> > > 
> > > Thanks for your suggestions.
> > > 
> > > Yes, it was doing as part of read() calls. But explicit ioctl to
> > > invalidate here. I was thinking like user space removing FD without
> > > reading or writing operation.
> > 
> > And is it possible to invalidate the dump without reading it fully
> > first?
> > 
> > If not then there is no point trying to do the call that is known to
> > fail anyway.
> 
> Generally not possible if uses librtas API rtas_platform_dump() which
> reads the dump completely and then the application calls this API
> explicitly to invalidate the dump (with buffer NULL - as doing in the
> current implementation). The current use case is extract_platdump
> command (ppc64-diag package)
> 
> extract_platdump() {   /* we are not chamging this implementation */
>  
> status = rtas_platform_dump() - initial call
> while !dump_complete {
> 	status = rtas_platform_dump()
> 	if (status < 0) failure
> 	if (status == 0) dump_complete = 1
> }
> 
> status = rtas_platform_dump() -  to invalidate dump by passing buffer =
> NULL
> 
> We should not expect using any command other than extract_platdump
> since the use case of collecting platform dump is narrow and is only on
> non-hmc based systems.
> 
> Hence added warning message if the dump is not completely read and
> invalidate the dump like removing file by mistake.
> 
> But I like your suggestion of returning an error (-EPERM) if not saved
> the dump completely.

I think EPERM is not correct in this case. It's not a problem of
permission but of incorrect state.

There are some errors around that like EBUSY or EINPROGRESS.

Thanks

Micahl