Re: [PATCH v6 26/28] cxl: add function for obtaining region range

[Alejandro Lucero Palau <alucerop@amd.com>]

On 12/9/24 16:29, Zhi Wang wrote:
> On Mon, 9 Dec 2024 09:48:01 +0000
> Alejandro Lucero Palau <alucerop@amd.com> wrote:
>
>> On 12/3/24 18:53, Zhi Wang wrote:
>>> On Mon, 2 Dec 2024 17:12:20 +0000
>>> <alejandro.lucero-palau@amd.com> wrote:
>>>
>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>
>>>> A CXL region struct contains the physical address to work with.
>>>>
>>>> Add a function for getting the cxl region range to be used for mapping
>>>> such memory range.
>>>>
>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>> ---
>>>>    drivers/cxl/core/region.c | 15 +++++++++++++++
>>>>    drivers/cxl/cxl.h         |  1 +
>>>>    include/cxl/cxl.h         |  1 +
>>>>    3 files changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>> index 5cb7991268ce..021e9b373cdd 100644
>>>> --- a/drivers/cxl/core/region.c
>>>> +++ b/drivers/cxl/core/region.c
>>>> @@ -2667,6 +2667,21 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
>>>>    	return ERR_PTR(rc);
>>>>    }
>>>>    
>>>> +int cxl_get_region_range(struct cxl_region *region, struct range *range)
>>>> +{
>>>> +	if (!region)
>>>> +		return -ENODEV;
>>>> +
>>> I am leaning towards having a WARN_ON_ONCE() above.
>>>
>> Not sure. The call is quite simple and to check the error should be
>> enough for understanding what is going on.
>>
> A sane caller would never calls this function with region == NULL. If
> that happens, it mostly means the caller itself has been problematic
> already, e.g. stack overflow. someone wrongly overwrites the pointer and
> the caller is not even aware of it. Thus it calls this function with
> region == NULL.
>
> In this case, we should not let it silently slip away. We should have
> WARN_ON or WARN_ON_ONCE to notify the admin that the system might be
> unstable now and some weird stuff happened and memory was
> randomly over-written.
>
> It is different from the second check, in which the caller is sane and get
> a error code.
>   


It makes sense.

I'll add it. I was close to send v7 ... :-)

A bit more of work but it does worth it.

Thanks!


>> In this case any error implies a problem with a previous call when
>> creating the region which was not likely checked for errors.
>>
>> And if a log is necessary, I think a WARN_ON should be used instead.
>>
>>
>>>> +	if (!region->params.res)
>>>> +		return -ENOSPC;
>>>> +
>>>> +	range->start = region->params.res->start;
>>>> +	range->end = region->params.res->end;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_NS_GPL(cxl_get_region_range, CXL);
>>>> +
>>>>    static ssize_t __create_region_show(struct cxl_root_decoder *cxlrd, char *buf)
>>>>    {
>>>>    	return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id));
>>>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>>>> index cc9e3d859fa6..32d2bd0520d4 100644
>>>> --- a/drivers/cxl/cxl.h
>>>> +++ b/drivers/cxl/cxl.h
>>>> @@ -920,6 +920,7 @@ void cxl_coordinates_combine(struct access_coordinate *out,
>>>>    
>>>>    bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
>>>>    
>>>> +int cxl_get_region_range(struct cxl_region *region, struct range *range);
>>>>    /*
>>>>     * Unit test builds overrides this to __weak, find the 'strong' version
>>>>     * of these symbols in tools/testing/cxl/.
>>>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>>>> index 14be26358f9c..0ed9e32f25dd 100644
>>>> --- a/include/cxl/cxl.h
>>>> +++ b/include/cxl/cxl.h
>>>> @@ -65,4 +65,5 @@ struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
>>>>    				     bool no_dax);
>>>>    
>>>>    int cxl_accel_region_detach(struct cxl_endpoint_decoder *cxled);
>>>> +int cxl_get_region_range(struct cxl_region *region, struct range *range);
>>>>    #endif