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

[Haren Myneni <haren@linux.ibm.com>]

On Thu, 2025-02-06 at 20:53 +0100, Michal Suchánek wrote:
> 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 for your suggestions - will use EINPROGRESS. 

> 
> Thanks
> 
> Micahl