diff mbox series

[v3,03/18] cxl/pci: cxl_hdm_decode_init: Move comment

Message ID 20250211095349.981096-4-rrichter@amd.com
State Superseded
Headers show
Series cxl: Address translation support, part 1: Cleanups and refactoring | expand

Commit Message

Robert Richter Feb. 11, 2025, 9:53 a.m. UTC 
The comment applies to the check, move it there.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/pci.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Jonathan Cameron Feb. 12, 2025, 6:09 p.m. UTC | #1 
On Tue, 11 Feb 2025 10:53:33 +0100
Robert Richter <rrichter@amd.com> wrote:

> The comment applies to the check, move it there.

I think I disagree. It was in the right place as far as I can tell.
It is an odd place for comment, but it's kind of describing
why it is not an error to get down there.

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/cxl/core/pci.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index f8e22bc278c3..c49efc419285 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -419,6 +419,15 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  	if (!hdm)
>  		return -ENODEV;
>  
> +	/*
> +	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> +	 * [High,Low] when HDM operation is enabled the range register values
> +	 * are ignored by the device, but the spec also recommends matching the
> +	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> +	 * are expected even though Linux does not require or maintain that
> +	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> +	 * Decoder Capability Enable.

This check is about mem_enabled. Would be fine to add another comment here to
say.

	/*
	 * If mem_enabled is not set prior configuration is irrelevant and we
	 * can do what we like so enable HDM decoders and ignore DVSEC registers.
	 */
> +	 */
>  	if (!info->mem_enabled) {
>  		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>  		if (rc)
> @@ -454,15 +463,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  		return -ENXIO;
>  	}
>  
> -	/*
> -	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> -	 * [High,Low] when HDM operation is enabled the range register values
> -	 * are ignored by the device, but the spec also recommends matching the
> -	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> -	 * are expected even though Linux does not require or maintain that
> -	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> -	 * Decoder Capability Enable.
> -	 */

This is the path the comment is talking about because only if we get to this
return path are we 'skipping' the HDM decoder capability and not returning
an error.  The path representing an HDM decoder equipped device that
was configured by a BIOS that decided to use the DVSEC registers.

I'm not sure why we care about how the hdm decoders are programmed inthis
case though.

I'm confused :(

>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, "CXL");
Robert Richter Feb. 13, 2025, 12:35 a.m. UTC | #2 
On 12.02.25 18:09:10, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:53:33 +0100
> Robert Richter <rrichter@amd.com> wrote:
> 
> > The comment applies to the check, move it there.
> 
> I think I disagree. It was in the right place as far as I can tell.
> It is an odd place for comment, but it's kind of describing
> why it is not an error to get down there.

Ah, that was not obvious to the reader. :-)

> 
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Reviewed-by: Gregory Price <gourry@gourry.net>
> > Tested-by: Gregory Price <gourry@gourry.net>
> > ---
> >  drivers/cxl/core/pci.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index f8e22bc278c3..c49efc419285 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -419,6 +419,15 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> >  	if (!hdm)
> >  		return -ENODEV;
> >  
> > +	/*
> > +	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > +	 * [High,Low] when HDM operation is enabled the range register values
> > +	 * are ignored by the device, but the spec also recommends matching the
> > +	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > +	 * are expected even though Linux does not require or maintain that
> > +	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > +	 * Decoder Capability Enable.
> 
> This check is about mem_enabled. Would be fine to add another comment here to
> say.

The next patch extends the comment for more clarification (I hope so).

> 
> 	/*
> 	 * If mem_enabled is not set prior configuration is irrelevant and we
> 	 * can do what we like so enable HDM decoders and ignore DVSEC registers.
> 	 */
> > +	 */
> >  	if (!info->mem_enabled) {
> >  		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> >  		if (rc)
> > @@ -454,15 +463,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> >  		return -ENXIO;
> >  	}
> >  
> > -	/*
> > -	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > -	 * [High,Low] when HDM operation is enabled the range register values
> > -	 * are ignored by the device, but the spec also recommends matching the
> > -	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > -	 * are expected even though Linux does not require or maintain that
> > -	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > -	 * Decoder Capability Enable.
> > -	 */
> 
> This is the path the comment is talking about because only if we get to this
> return path are we 'skipping' the HDM decoder capability and not returning
> an error.  The path representing an HDM decoder equipped device that
> was configured by a BIOS that decided to use the DVSEC registers.
> 
> I'm not sure why we care about how the hdm decoders are programmed inthis
> case though.
> 
> I'm confused :(

There is an HDM, but it is disabled (CXL_HDM_DECODER_ENABLE is
cleared). If the DVSEC range regs do not have valid values
(!info->mem_enabled, firmware indicates it is not used), just go and
enable the HDM.

We try to use the hdm decoders here to be able to use them for a
non-auto setup. Else, decoder emulation is used
(should_emulate_decoders()) and decoders are locked
(CXL_DECODER_F_LOCK will be set).

Maybe take a look at the whole change with added comments including
patch 4/18?

I hope to not add confusion here. :-)

-Robert

> 
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, "CXL");
>
Jonathan Cameron Feb. 14, 2025, 3:49 p.m. UTC | #3 
On Thu, 13 Feb 2025 01:35:29 +0100
Robert Richter <rrichter@amd.com> wrote:

> On 12.02.25 18:09:10, Jonathan Cameron wrote:
> > On Tue, 11 Feb 2025 10:53:33 +0100
> > Robert Richter <rrichter@amd.com> wrote:
> >   
> > > The comment applies to the check, move it there.  
> > 
> > I think I disagree. It was in the right place as far as I can tell.
> > It is an odd place for comment, but it's kind of describing
> > why it is not an error to get down there.  
> 
> Ah, that was not obvious to the reader. :-)
> 
> >   
> > > 
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > Reviewed-by: Gregory Price <gourry@gourry.net>
> > > Tested-by: Gregory Price <gourry@gourry.net>
> > > ---
> > >  drivers/cxl/core/pci.c | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > > index f8e22bc278c3..c49efc419285 100644
> > > --- a/drivers/cxl/core/pci.c
> > > +++ b/drivers/cxl/core/pci.c
> > > @@ -419,6 +419,15 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> > >  	if (!hdm)
> > >  		return -ENODEV;
> > >  
> > > +	/*
> > > +	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > > +	 * [High,Low] when HDM operation is enabled the range register values
> > > +	 * are ignored by the device, but the spec also recommends matching the
> > > +	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > > +	 * are expected even though Linux does not require or maintain that
> > > +	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > > +	 * Decoder Capability Enable.  
> > 
> > This check is about mem_enabled. Would be fine to add another comment here to
> > say.  
> 
> The next patch extends the comment for more clarification (I hope so).

Not to me.  It says 'else' when referring to what happens in the if.

> 
> > 
> > 	/*
> > 	 * If mem_enabled is not set prior configuration is irrelevant and we
> > 	 * can do what we like so enable HDM decoders and ignore DVSEC registers.
> > 	 */  
> > > +	 */
> > >  	if (!info->mem_enabled) {
> > >  		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> > >  		if (rc)
> > > @@ -454,15 +463,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> > >  		return -ENXIO;
> > >  	}
> > >  
> > > -	/*
> > > -	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > > -	 * [High,Low] when HDM operation is enabled the range register values
> > > -	 * are ignored by the device, but the spec also recommends matching the
> > > -	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > > -	 * are expected even though Linux does not require or maintain that
> > > -	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > > -	 * Decoder Capability Enable.
> > > -	 */  
> > 
> > This is the path the comment is talking about because only if we get to this
> > return path are we 'skipping' the HDM decoder capability and not returning
> > an error.  The path representing an HDM decoder equipped device that
> > was configured by a BIOS that decided to use the DVSEC registers.
> > 
> > I'm not sure why we care about how the hdm decoders are programmed inthis
> > case though.
> > 
> > I'm confused :(  
> 
> There is an HDM, but it is disabled (CXL_HDM_DECODER_ENABLE is
> cleared). If the DVSEC range regs do not have valid values
> (!info->mem_enabled, firmware indicates it is not used), just go and
> enable the HDM.
> 
> We try to use the hdm decoders here to be able to use them for a
> non-auto setup. Else, decoder emulation is used
> (should_emulate_decoders()) and decoders are locked
> (CXL_DECODER_F_LOCK will be set).
> 
> Maybe take a look at the whole change with added comments including
> patch 4/18?
> 
> I hope to not add confusion here. :-)
> 
> -Robert
> 
> >   
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, "CXL");  
> >
Robert Richter March 6, 2025, 9:38 a.m. UTC | #4 
On 14.02.25 15:49:55, Jonathan Cameron wrote:
> On Thu, 13 Feb 2025 01:35:29 +0100
> Robert Richter <rrichter@amd.com> wrote:
> > On 12.02.25 18:09:10, Jonathan Cameron wrote:
> > > On Tue, 11 Feb 2025 10:53:33 +0100
> > > Robert Richter <rrichter@amd.com> wrote:

> > > > +	/*
> > > > +	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > > > +	 * [High,Low] when HDM operation is enabled the range register values
> > > > +	 * are ignored by the device, but the spec also recommends matching the
> > > > +	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > > > +	 * are expected even though Linux does not require or maintain that
> > > > +	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > > > +	 * Decoder Capability Enable.  
> > > 
> > > This check is about mem_enabled. Would be fine to add another comment here to
> > > say.  
> > 
> > The next patch extends the comment for more clarification (I hope so).
> 
> Not to me.  It says 'else' when referring to what happens in the if.

I have dropped this patch and updated the comments in the next patch
along with the patch description.

-Robert
diff mbox series

Patch

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index f8e22bc278c3..c49efc419285 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -419,6 +419,15 @@  int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	if (!hdm)
 		return -ENODEV;
 
+	/*
+	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
+	 * [High,Low] when HDM operation is enabled the range register values
+	 * are ignored by the device, but the spec also recommends matching the
+	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
+	 * are expected even though Linux does not require or maintain that
+	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
+	 * Decoder Capability Enable.
+	 */
 	if (!info->mem_enabled) {
 		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
 		if (rc)
@@ -454,15 +463,6 @@  int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 		return -ENXIO;
 	}
 
-	/*
-	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
-	 * [High,Low] when HDM operation is enabled the range register values
-	 * are ignored by the device, but the spec also recommends matching the
-	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
-	 * are expected even though Linux does not require or maintain that
-	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
-	 * Decoder Capability Enable.
-	 */
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, "CXL");