Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Date:   Mon, 27 Nov 2023 15:40:57 +0530
From:   Pavan Kondeti <quic_pkondeti@quicinc.com>
To:     Mukesh Ojha <quic_mojha@quicinc.com>
CC:     <corbet@lwn.net>, <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <keescook@chromium.org>, <tony.luck@intel.com>,
        <gpiccoli@igalia.com>, <mathieu.poirier@linaro.org>,
        <vigneshr@ti.com>, <nm@ti.com>, <matthias.bgg@gmail.com>,
        <kgene@kernel.org>, <alim.akhtar@samsung.com>,
        <bmasney@redhat.com>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-hardening@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-mediatek@lists.infradead.org>,
        <linux-samsung-soc@vger.kernel.org>, <kernel@quicinc.com>
Subject: Re: [Patch v6 11/12] pstore/ram: Add ramoops ready notifier support
Message-ID: <3636dc3a-b62b-4ff9-bdc3-fec496a804b7@quicinc.com>
References: <1700864395-1479-1-git-send-email-quic_mojha@quicinc.com>
 <1700864395-1479-12-git-send-email-quic_mojha@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <1700864395-1479-12-git-send-email-quic_mojha@quicinc.com>
Precedence: bulk

On Sat, Nov 25, 2023 at 03:49:54AM +0530, Mukesh Ojha wrote:
> Client like minidump, is only interested in ramoops
> region addresses/size so that it could register them
> with its table and also it is only deals with ram
> backend and does not use pstorefs to read the records.
> Let's introduce a client notifier in ramoops which
> gets called when ramoops driver probes successfully
> and it passes the ramoops region information to the
> passed callback by the client and If the call for
> ramoops ready register comes after ramoops probe
> than call the callback directly.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  fs/pstore/ram.c            | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pstore_ram.h |  6 ++++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index a6c0da8cfdd4..72341fd21aec 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -22,6 +22,7 @@
>  #include <linux/of_address.h>
>  #include <linux/memblock.h>
>  #include <linux/mm.h>
> +#include <linux/mutex.h>
>  
>  #include "internal.h"
>  #include "ram_internal.h"
> @@ -101,6 +102,14 @@ struct ramoops_context {
>  	unsigned int ftrace_read_cnt;
>  	unsigned int pmsg_read_cnt;
>  	struct pstore_info pstore;
> +	/*
> +	 * Lock to serialize calls to register_ramoops_ready_notifier,
> +	 * ramoops_ready_notifier and read/modification of 'ramoops_ready'.
> +	 */
> +	struct mutex lock;
> +	bool ramoops_ready;
> +	int (*callback)(const char *name, int id, void *vaddr,
> +			phys_addr_t paddr, size_t size);
>  };
>  
>  static struct platform_device *dummy;
> @@ -488,6 +497,7 @@ static int ramoops_pstore_erase(struct pstore_record *record)
>  }
>  
>  static struct ramoops_context oops_cxt = {
> +	.lock   = __MUTEX_INITIALIZER(oops_cxt.lock),
>  	.pstore = {
>  		.owner	= THIS_MODULE,
>  		.name	= "ramoops",
> @@ -662,6 +672,68 @@ static int ramoops_init_prz(const char *name,
>  	return 0;
>  }
>  
> +void ramoops_ready_notifier(struct ramoops_context *cxt)
> +{
> +	struct persistent_ram_zone *prz;
> +	int i;
> +
> +	if (!cxt->callback)
> +		return;
> +
> +	for (i = 0; i < cxt->max_dump_cnt; i++) {
> +		prz = cxt->dprzs[i];
> +		cxt->callback("dmesg", i, prz->vaddr, prz->paddr, prz->size);
> +	}
> +
> +	if (cxt->console_size) {
> +		prz = cxt->cprz;
> +		cxt->callback("console", 0, prz->vaddr, prz->paddr, prz->size);
> +	}
> +
> +	for (i = 0; i < cxt->max_ftrace_cnt; i++) {
> +		prz = cxt->fprzs[i];
> +		cxt->callback("ftrace", i, prz->vaddr, prz->paddr, prz->size);
> +	}
> +
> +	if (cxt->pmsg_size) {
> +		prz = cxt->mprz;
> +		cxt->callback("pmsg", 0, prz->vaddr, prz->paddr, prz->size);
> +	}
> +}
> +
> +int register_ramoops_ready_notifier(int (*fn)(const char *, int,
> +				   void *, phys_addr_t, size_t))
> +{
> +	struct ramoops_context *cxt = &oops_cxt;
> +
> +	mutex_lock(&cxt->lock);
> +	if (cxt->callback) {
> +		mutex_unlock(&cxt->lock);
> +		return -EEXIST;
> +	}
> +
> +	cxt->callback = fn;
> +	if (cxt->ramoops_ready)
> +		ramoops_ready_notifier(cxt);
> +
> +	mutex_unlock(&cxt->lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(register_ramoops_ready_notifier);
> +

Can you please elaborate on why do we need this custom notifier logic? 

why would not a standard notifier (include/linux/notifier.h) work here? 
The notifier_call callback can recieve custom data from the 
notifier chain implementer. All we need is to define a custom struct like

struct pstore_ramoops_zone_data {
	const char *name;
	int id;
	void *vaddr;
	phys_addr_t paddr;
	size_t size;
};

and pass the pointer to array of this struct. 


btw, the current logic only supports just one client and this limitation
is not highlighted any where.

Thanks,
Pavan