Re: [PATCH 20/23] cxl/port: Introduce a port driver

[Ben Widawsky <ben.widawsky@intel.com>]

On 21-11-23 11:38:23, Jonathan Cameron wrote:
> On Mon, 22 Nov 2021 15:38:20 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> ...
> 
> > > > +static int enumerate_hdm_decoders(struct cxl_port *port,
> > > > +				  struct cxl_port_data *portdata)
> > > > +{
> > > > +	void __iomem *hdm_decoder = portdata->regs.hdm_decoder;
> > > > +	bool global_enable;
> > > > +	u32 global_ctrl;
> > > > +	int i = 0;
> > > > +
> > > > +	global_ctrl = readl(hdm_decoder + CXL_HDM_DECODER_CTRL_OFFSET);
> > > > +	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
> > > > +	if (!global_enable) {
> > > > +		i = dvsec_range_used(port);
> > > > +		if (i) {
> > > > +			dev_err(&port->dev,
> > > > +				"Couldn't add port because device is using DVSEC range registers\n");
> > > > +			return -EBUSY;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < portdata->caps.count; i++) {
> > > > +		int rc, target_count = portdata->caps.tc;
> > > > +		struct cxl_decoder *cxld;
> > > > +		int *target_map = NULL;
> > > > +		u64 size;
> > > > +
> > > > +		if (is_endpoint_port(port))
> > > > +			target_count = 0;
> > > > +
> > > > +		cxld = cxl_decoder_alloc(port, target_count);
> > > > +		if (IS_ERR(cxld)) {
> > > > +			dev_warn(&port->dev,
> > > > +				 "Failed to allocate the decoder\n");
> > > > +			return PTR_ERR(cxld);
> > > > +		}
> > > > +
> > > > +		cxld->target_type = CXL_DECODER_EXPANDER;
> > > > +		cxld->interleave_ways = 1;
> > > > +		cxld->interleave_granularity = 0;
> > > > +
> > > > +		size = get_decoder_size(hdm_decoder, i);
> > > > +		if (size != 0) {
> > > > +#define decoderN(reg, n) hdm_decoder + CXL_HDM_DECODER0_##reg(n)  
> > > 
> > > Perhaps this block in the if (size != 0) would be more readable if broken out
> > > to a utility function?  
> > 
> > I don't get this comment, there is already get_decoder_size(). Can you please
> > elaborate?
> 
> Sure.  Just talking about having something like
> 
> 		if (size != 0)
> 			init_decoder() // or something better named
> 
> as an alternative to this deep nesting. 
> 

Sounds good. I can combine it with the similar initialization done in cxl_acpi.

> > 
> > > As normal I'm not keen on the macro magic if we can avoid it.
> > >   
> > 
> > Yeah - just trying to not have to deal with wrapping long lines.
> > 
> > >   
> > > > +			int temp[CXL_DECODER_MAX_INTERLEAVE];
> > > > +			u64 base;
> > > > +			u32 ctrl;
> > > > +			int j;
> > > > +			union {
> > > > +				u64 value;
> > > > +				char target_id[8];
> > > > +			} target_list;  
> > > 
> > > I thought we tried to avoid this union usage in kernel because of the whole
> > > thing about c compilers being able to do what they like with it...
> > >   
> > 
> > I wasn't aware of the restriction. I can change it back if it's required. It
> > does look a lot nicer this way. Is there a reference to this issue somewhere?
> 
> Hmm. Seems I was out of date on this.  There is a mess in the c99 standard that
> contradicts itself on whether you can do this or not.
> 
> https://davmac.wordpress.com/2010/02/26/c99-revisited/

Thanks for the link.

> 
> The pull request for a patch form Andy got a Linus response...
> 
> https://lore.kernel.org/all/CAJZ5v0jq45atkapwSjJ4DkHhB1bfOA-Sh1TiA3dPXwKyFTBheA@mail.gmail.com/
> 

That was a fun read :-)

I'll defer to Dan on this. This was actually his code that he suggested in
review of the RFC.

> 
> > 
> > > > +
> > > > +			target_map = temp;  
> > > 
> > > This is set to a variable that goes out of scope whilst target_map is still in
> > > use.
> > >   
> > 
> > Yikes. I'm pretty surprised the compiler didn't catch this.
> > 
> > > > +			ctrl = readl(decoderN(CTRL_OFFSET, i));
> > > > +			base = ioread64_hi_lo(decoderN(BASE_LOW_OFFSET, i));
> > > > +
> > > > +			cxld->decoder_range = (struct range){
> > > > +				.start = base,
> > > > +				.end = base + size - 1
> > > > +			};
> > > > +
> > > > +			cxld->flags = CXL_DECODER_F_ENABLE;
> > > > +			cxld->interleave_ways = to_interleave_ways(ctrl);
> > > > +			cxld->interleave_granularity =
> > > > +				to_interleave_granularity(ctrl);
> > > > +
> > > > +			if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0)
> > > > +				cxld->target_type = CXL_DECODER_ACCELERATOR;
> > > > +
> > > > +			target_list.value = ioread64_hi_lo(decoderN(TL_LOW, i));
> > > > +			for (j = 0; j < cxld->interleave_ways; j++)
> > > > +				target_map[j] = target_list.target_id[j];
> > > > +#undef decoderN
> > > > +		}
> > > > +
> > > > +		rc = cxl_decoder_add_locked(cxld, target_map);
> > > > +		if (rc)
> > > > +			put_device(&cxld->dev);
> > > > +		else
> > > > +			rc = cxl_decoder_autoremove(&port->dev, cxld);
> > > > +		if (rc)
> > > > +			dev_err(&port->dev, "Failed to add decoder\n");  
> > > 
> > > If that fails on the autoremove registration (unlikely) this message
> > > will be rather confusing - as the add was fine...
> > > 
> > > This nest of carrying on when we have an error is getting ever deeper...
> > >   
> > 
> > Yeah, this is not great. I will clean it up.
> > 
> > Thanks.
> > 
> > > > +		else
> > > > +			dev_dbg(&cxld->dev, "Added to port %s\n",
> > > > +				dev_name(&port->dev));
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Turn on global enable now since DVSEC ranges aren't being used and
> > > > +	 * we'll eventually want the decoder enabled.
> > > > +	 */
> > > > +	if (!global_enable) {
> > > > +		dev_dbg(&port->dev, "Enabling HDM decode\n");
> > > > +		writel(global_ctrl | CXL_HDM_DECODER_ENABLE, hdm_decoder + CXL_HDM_DECODER_CTRL_OFFSET);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +  
>