Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <a869ec2e-966b-f8ea-7f68-a77c214a45af@quicinc.com>
Date:   Mon, 9 Oct 2023 17:29:40 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [REBASE PATCH v5 10/17] pstore: Add pstore_region_defined()
 helper and export it
To:     Kees Cook <keescook@chromium.org>
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>,
        <tony.luck@intel.com>, <gpiccoli@igalia.com>,
        <mathieu.poirier@linaro.org>, <catalin.marinas@arm.com>,
        <will@kernel.org>, <linus.walleij@linaro.org>,
        <andy.shevchenko@gmail.com>, <vigneshr@ti.com>, <nm@ti.com>,
        <matthias.bgg@gmail.com>, <kgene@kernel.org>,
        <alim.akhtar@samsung.com>, <bmasney@redhat.com>,
        <quic_tsoni@quicinc.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-gpio@vger.kernel.org>, <linux-mediatek@lists.infradead.org>,
        <linux-samsung-soc@vger.kernel.org>, <kernel@quicinc.com>
References: <1694429639-21484-1-git-send-email-quic_mojha@quicinc.com>
 <1694429639-21484-11-git-send-email-quic_mojha@quicinc.com>
 <202309131620.34EB0F6972@keescook>
Content-Language: en-US
From:   Mukesh Ojha <quic_mojha@quicinc.com>
In-Reply-To: <202309131620.34EB0F6972@keescook>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Thanks for the review.

On 9/14/2023 4:54 AM, Kees Cook wrote:
> On Mon, Sep 11, 2023 at 04:23:52PM +0530, Mukesh Ojha wrote:
>> There are users like Qualcomm minidump which is interested in
>> knowing the pstore frontend addresses and sizes from the backend
>> (ram) to be able to register it with firmware to finally collect
>> them during crash for debugging.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   fs/pstore/platform.c   | 15 +++++++++++++++
>>   fs/pstore/ram.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pstore.h |  6 ++++++
>>   3 files changed, 63 insertions(+)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index e5bca9a004cc..fdac951059c1 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -139,6 +139,21 @@ enum pstore_type_id pstore_name_to_type(const char *name)
>>   }
>>   EXPORT_SYMBOL_GPL(pstore_name_to_type);
>>   
>> +int pstore_region_defined(struct pstore_record *record,
>> +			  void **virt, phys_addr_t *phys,
>> +			  size_t *size, unsigned int *max_dump_cnt)
>> +{
>> +	if (!psinfo)
>> +		return -EINVAL;
>> +
>> +	record->psi = psinfo;
> 
> Uh, this makes no sense to me. If this is a real pstore_record, you
> cannot just assign psi here.

Ok.

> 
>> +
>> +	return psinfo->region_info ?
>> +	       psinfo->region_info(record, virt, phys, size, max_dump_cnt) :
>> +	       -EINVAL;
> 
> Common code style for this kind of thing is usually like this:
> 
> 	if (!psinfo->region_info)
> 		return -EINVAL;
> 
> 	return psinfo->region_info(...)

Thanks.

-Mukesh
> 
>> +}
>> +EXPORT_SYMBOL_GPL(pstore_region_defined);
>> +
>>   static void pstore_timer_kick(void)
>>   {
>>   	if (pstore_update_ms < 0)
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index ab551caa1d2a..62202f3ddf63 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -437,6 +437,47 @@ static int ramoops_pstore_erase(struct pstore_record *record)
>>   	return 0;
>>   }
>>   
>> +static int ramoops_region_info(struct pstore_record *record,
>> +			       void **virt, phys_addr_t *phys,
>> +			       size_t *size, unsigned int *max_dump_cnt)
> 
> But there's a larger problem here -- "virt", "phys" and likely
> "max_dump_cnt" are aspects _specific to the ram backend_. This can't be
> a generic pstore interface.
> 
> I'm not opposed to it being exposed only from ramoops, though.
> 
> But I think you'll want a pstore generic way to iterate over the
> records..