Re: [PATCH v4 04/21] soc: qcom: Add Qualcomm APSS minidump (frontend) feature support

[Pavan Kondeti <quic_pkondeti@xxxxxxxxxxx>]

On Fri, Jun 30, 2023 at 12:45:13PM +0530, Mukesh Ojha wrote:
> > > +/**
> > > + * qcom_minidump_region_register() - Register region in APSS Minidump table.
> > > + * @region: minidump region.
> > > + *
> > > + * Return: On success, it returns 0 and negative error value on failure.
> > > + */
> > 
> > Are we not going to return any cookie upon success which can be passed
> > to us during unregistration?
> 
> e.g, Let's just say if we return the region number as an cookies, the
> problem i see that, we multiple unregistration is happening we will
> be shifting the array upwards and that results in the index/cookies does
> not retain the same for the shifted regions.
> 
> So, at the moment, client need to pass the same region for un-registration
> as they have passed for registration..
> 

>From a driver user pov, there is no reason for keeping
qcom_minidump_region struct around after the registration, agree?
However, the unregistration API expects it again, so the driver needs to
cache it. The region is no way being used by the minidump core either..so it is
just there for no reason. Hence I asked this question.

Thanks for the explanation about why the region number can't be used as
cookie. Is it a limitation on the firmware that we need to left shift
all regions when a region is deleted? I ask this because
minidump_region::valid is avaialble for Linux to mark a lazy deletion.
Sure your look up would have to traverse the entire array in worst case,
but today also you are doing that for duplicate name check.
If this lazy deletion can be implemented, the region number can be used
a cookie, right?

> > 
> > > +int qcom_minidump_region_register(const struct qcom_minidump_region *region)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!qcom_minidump_valid_region(region))
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&md_lock);
> > > +	if (!md) {
> > > +		mutex_unlock(&md_lock);
> > > +		pr_err("No backend registered yet, try again..");
> > > +		return -EPROBE_DEFER;
> > > +	}
> > > +
> > > +	ret = md->ops->md_region_register(md, region);
> > > +	if (ret)
> > > +		goto unlock;
> > > +
> > 
> > The md_lock description in the beginning does not seems to be correct.
> > It is not limited to backend registration. It has much wider usage.
> > 
> > You are holding the lock while calling into backend. Basically the
> > synchronization is done in the front end.
> 
> Initially, the thought was to have the backend their own lock but that
> is irrelevant as all the contention is there in the front end.
> 
> So, used the same lock for synchronization as i moved in developement
> and the later that comment became obsolete.
> 
> Thanks for catching this.
> will fix the comment.
> 

Sure

> > 
> > > +	qcom_md_update_elf_header(region);
> > > +unlock:
> > > +	mutex_unlock(&md_lock);
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
> 
> > 
> > > +	ret = qcom_minidump_clear_header(region);
> > > +unlock:
> > > +	mutex_unlock(&md_lock);
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(qcom_minidump_region_unregister);
> > 
> > can we create a namespace for exporting these symbols?
> 
> Any specific reason, you are suggesting this ?
> 

My original intention was to mark qcom_minidump_backend_register()
and qcom_minidump_backend_unregister() under a namespace. Because these
are not meant for any drivers but only for minidump backend. That serves
as a clear documentation that minidump implementation is spanned across
this frontend and backend modules.

Thanks,
Pavan