Re: [PATCH v4 21/28] cxl/extent: Process DCD events and realize region extents

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 9 Oct 2024 14:49:09 -0500
Ira Weiny <ira.weiny@intel.com> wrote:

> Li, Ming4 wrote:
> > On 10/8/2024 7:16 AM, ira.weiny@intel.com wrote:  
> > > From: Navneet Singh <navneet.singh@intel.com>
> > >  
> 
> [snip]
> 
> > >
> > > Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> > > Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > >  
> > Hi Ira,
> > 
> > I guess you missed my comments for V3, I comment it again for this patch.  
> 
> Apologies.  Yes I totally missed your reply.  :-(
> 
> >   
> > > +static bool extents_contain(struct cxl_dax_region *cxlr_dax,
> > > +			    struct cxl_endpoint_decoder *cxled,
> > > +			    struct range *new_range)
> > > +{
> > > +	struct device *extent_device;
> > > +	struct match_data md = {
> > > +		.cxled = cxled,
> > > +		.new_range = new_range,
> > > +	};
> > > +
> > > +	extent_device = device_find_child(&cxlr_dax->dev, &md, match_contains);
> > > +	if (!extent_device)
> > > +		return false;
> > > +
> > > +	put_device(extent_device);  
> > could use __free(put_device) to drop this 'put_device(extent_device)'  
> 
> Yep.
> 
> > > +	return true;
> > > +}  
> > [...]  
> > > +static bool extents_overlap(struct cxl_dax_region *cxlr_dax,
> > > +			    struct cxl_endpoint_decoder *cxled,
> > > +			    struct range *new_range)
> > > +{
> > > +	struct device *extent_device;
> > > +	struct match_data md = {
> > > +		.cxled = cxled,
> > > +		.new_range = new_range,
> > > +	};
> > > +
> > > +	extent_device = device_find_child(&cxlr_dax->dev, &md, match_overlaps);
> > > +	if (!extent_device)
> > > +		return false;
> > > +
> > > +	put_device(extent_device);  
> > Same as above.  
> 
> Done.
> 
> > > +	return true;
> > > +}
> > > +  
> > [...]  
> > > +static int cxl_send_dc_response(struct cxl_memdev_state *mds, int opcode,
> > > +				struct xarray *extent_array, int cnt)
> > > +{
> > > +	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
> > > +	struct cxl_mbox_dc_response *p;
> > > +	struct cxl_mbox_cmd mbox_cmd;
> > > +	struct cxl_extent *extent;
> > > +	unsigned long index;
> > > +	u32 pl_index;
> > > +	int rc;
> > > +
> > > +	size_t pl_size = struct_size(p, extent_list, cnt);
> > > +	u32 max_extents = cnt;
> > > +
> > > +	/* May have to use more bit on response. */
> > > +	if (pl_size > cxl_mbox->payload_size) {
> > > +		max_extents = (cxl_mbox->payload_size - sizeof(*p)) /
> > > +			      sizeof(struct updated_extent_list);
> > > +		pl_size = struct_size(p, extent_list, max_extents);
> > > +	}
> > > +
> > > +	struct cxl_mbox_dc_response *response __free(kfree) =
> > > +						kzalloc(pl_size, GFP_KERNEL);
> > > +	if (!response)
> > > +		return -ENOMEM;
> > > +
> > > +	pl_index = 0;
> > > +	xa_for_each(extent_array, index, extent) {
> > > +
> > > +		response->extent_list[pl_index].dpa_start = extent->start_dpa;
> > > +		response->extent_list[pl_index].length = extent->length;
> > > +		pl_index++;
> > > +		response->extent_list_size = cpu_to_le32(pl_index);
> > > +
> > > +		if (pl_index == max_extents) {
> > > +			mbox_cmd = (struct cxl_mbox_cmd) {
> > > +				.opcode = opcode,
> > > +				.size_in = struct_size(response, extent_list,
> > > +						       pl_index),
> > > +				.payload_in = response,
> > > +			};
> > > +
> > > +			response->flags = 0;
> > > +			if (pl_index < cnt)
> > > +				response->flags &= CXL_DCD_EVENT_MORE;  
> > 
> > It should be 'response->flags |= CXL_DCD_EVENT_MORE' here.  
> 
> Ah yea.  Good catch.
> 
> > 
> > Another issue is if 'cnt' is N times bigger than 'max_extents'(e,g. cnt=20, max_extents=10). all responses will be sent in this xa_for_each(), and CXL_DCD_EVENT_MORE will be set in the last response but it should not be set in these cases.
> >   
> 
> Ah yes.  cnt must be decremented.  As I looked at the patch just now the
> 
> 	if (cnt == 0 || pl_index)
> 
> ... seemed very wrong to me.  That change masked this bug.
> 
> This should fix it:
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d66beec687a0..99200274dea8 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1119,10 +1119,11 @@ static int cxl_send_dc_response(struct cxl_memdev_state *mds, int opcode,
>                         if (rc)
>                                 return rc;
>                         pl_index = 0;
> +                       cnt -= pl_index;
>                 }
>         }
>  
> -       if (cnt == 0 || pl_index) {

I thought this cnt == 0 check was to deal with the no valid
extents case where an empty reply is needed.


> +       if (pl_index) {
>                 mbox_cmd = (struct cxl_mbox_cmd) {
>                         .opcode = opcode,
>                         .size_in = struct_size(response, extent_list,
> 
> 
> Thank you, and sorry again for missing your feedback.
> 
> Ira
> 
> [snip]
>