[PATCH] fpga: altera-cvp: Truncated bitstream error support

[PATCH] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

From: Ang Tien Sung <tien.sung.ang@intel.com>

To support the error handling of a truncated bitstream sent.
The current AIB CvP firmware is not capable of handling a
data stream smaller than 4096bytes. The firmware's limitation
causes a hung-up as it's DMA engine waits forever for the
completion of the instructed 4096bytes.
To resolve this design limitation, both firmware and CvP
driver made several changes. At the CvP driver, we just
have to ensure that anything lesser than 4096bytes are
padded with extra bytes. The CvP will then, initiate the
tear-down by clearing the START_XFER and CVP_CONFIG bits.
We should also check for CVP_ERROR during the CvP completion.
A send_buf which is always 4096bytes is used to copy the
data during every transaction.

Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
---
 drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index 4ffb9da537d8..80edcfb5e5fc 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -81,6 +81,7 @@ struct altera_cvp_conf {
 	u8			numclks;
 	u32			sent_packets;
 	u32			vsec_offset;
+	u8			*send_buf;
 	const struct cvp_priv	*priv;
 };
 
@@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
 		}
 
 		len = min(conf->priv->block_size, remaining);
-		altera_cvp_send_block(conf, data, len);
+		/* Copy the requested host data into the transmit buffer */
+
+		memcpy(conf->send_buf, data, len);
+		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
+		conf->priv->block_size);
 		data += len / sizeof(u32);
 		done += len;
 		remaining -= len;
@@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
 	if (ret)
 		return ret;
 
-	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
-	altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val);
-	if (val & VSE_UNCOR_ERR_CVP_CFG_ERR) {
-		dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n");
+	/*
+	 * STEP 16 - If bitstream error (truncated/miss-matched),
+	 * we shall exit here.
+	 */
+	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
+	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
+		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");
 		return -EPROTO;
 	}
 
@@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 
 	pci_set_drvdata(pdev, mgr);
 
+	/* Allocate the 4096 block size transmit buffer */
+	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);
+	if (!conf->send_buf) {
+		ret = -ENOMEM;
+		goto err_unmap;
+	}
 	return 0;
 
 err_unmap:
-- 
2.25.1

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Tom Rix]

On 5/17/22 11:48 PM, tien.sung.ang@intel.com wrote:
> From: Ang Tien Sung <tien.sung.ang@intel.com>
>
> To support the error handling of a truncated bitstream sent.
> The current AIB CvP firmware is not capable of handling a
> data stream smaller than 4096bytes. The firmware's limitation
> causes a hung-up as it's DMA engine waits forever for the
> completion of the instructed 4096bytes.
> To resolve this design limitation, both firmware and CvP
> driver made several changes. At the CvP driver, we just
> have to ensure that anything lesser than 4096bytes are
> padded with extra bytes. The CvP will then, initiate the
> tear-down by clearing the START_XFER and CVP_CONFIG bits.
> We should also check for CVP_ERROR during the CvP completion.
> A send_buf which is always 4096bytes is used to copy the
> data during every transaction.
>
> Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> ---
>   drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 4ffb9da537d8..80edcfb5e5fc 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -81,6 +81,7 @@ struct altera_cvp_conf {
>   	u8			numclks;
>   	u32			sent_packets;
>   	u32			vsec_offset;
> +	u8			*send_buf;

Why is it necessary to hold  the send_buf in this structure ?

If it is used only in *_write, could it alloc/freed there ?

Because the write happens rarely, my preference is to alloc/free in 
*_write().

>   	const struct cvp_priv	*priv;
>   };
>   
> @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
>   		}
>   
>   		len = min(conf->priv->block_size, remaining);
> -		altera_cvp_send_block(conf, data, len);
> +		/* Copy the requested host data into the transmit buffer */
> +
> +		memcpy(conf->send_buf, data, len);
Is a memset needed for a short buffer ?
> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> +		conf->priv->block_size);
>   		data += len / sizeof(u32);
>   		done += len;
>   		remaining -= len;
> @@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
>   	if (ret)
>   		return ret;
>   
> -	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
> -	altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val);
> -	if (val & VSE_UNCOR_ERR_CVP_CFG_ERR) {
> -		dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n");
> +	/*
> +	 * STEP 16 - If bitstream error (truncated/miss-matched),
> +	 * we shall exit here.
> +	 */
> +	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
Should this be STEP 17 ? the old 16 checked something else.

Tom

> +	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
> +		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");
>   		return -EPROTO;
>   	}
>   
> @@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev,
>   
>   	pci_set_drvdata(pdev, mgr);
>   
> +	/* Allocate the 4096 block size transmit buffer */
> +	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);
> +	if (!conf->send_buf) {
> +		ret = -ENOMEM;
> +		goto err_unmap;
> +	}
>   	return 0;
>   
>   err_unmap:

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Christophe JAILLET]

Le 18/05/2022 à 08:48, tien.sung.ang@intel.com a écrit :
> From: Ang Tien Sung <tien.sung.ang@intel.com>
> 
> To support the error handling of a truncated bitstream sent.
> The current AIB CvP firmware is not capable of handling a
> data stream smaller than 4096bytes. The firmware's limitation
> causes a hung-up as it's DMA engine waits forever for the
> completion of the instructed 4096bytes.
> To resolve this design limitation, both firmware and CvP
> driver made several changes. At the CvP driver, we just
> have to ensure that anything lesser than 4096bytes are
> padded with extra bytes. The CvP will then, initiate the
> tear-down by clearing the START_XFER and CVP_CONFIG bits.
> We should also check for CVP_ERROR during the CvP completion.
> A send_buf which is always 4096bytes is used to copy the
> data during every transaction.
> 
> Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> ---
>   drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 4ffb9da537d8..80edcfb5e5fc 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -81,6 +81,7 @@ struct altera_cvp_conf {
>   	u8			numclks;
>   	u32			sent_packets;
>   	u32			vsec_offset;
> +	u8			*send_buf;
>   	const struct cvp_priv	*priv;
>   };
>   
> @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
>   		}
>   
>   		len = min(conf->priv->block_size, remaining);
> -		altera_cvp_send_block(conf, data, len);
> +		/* Copy the requested host data into the transmit buffer */
> +
> +		memcpy(conf->send_buf, data, len);
> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> +		conf->priv->block_size);

Nitpicking: this should be aligned with 'conf'.

CJ

>   		data += len / sizeof(u32);
>   		done += len;
>   		remaining -= len;

[...]

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

Thanks for pointing that out. It is always good to get all the alignments right.
I will add this to the next revision of the patch.

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

The send_buf is always used throughout the life-span of the CvP driver.
Hence, we thought it would be wise to just pre-allocate it one time
at the start of the probe/init.
It is also fine if we do it in the altera_cvp_write. The only issue we
see in this is that, a minor hit on the performance as you need to 
then, allocate the buffer on every new CvP FPGA configuration write.

As for STEP 16, the previous implementation checks the Error latch bit
which stores the previous transaction's result. If an error occurs
prior to this, the driver would throw an error which is not right.
The correct step is to just check for the current CvP error status 
from the register.
Hope that is fine with you. Thanks

[PATCH v2] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

From: Ang Tien Sung <tien.sung.ang@intel.com>

To support the error handling of a truncated bitstream sent.
The current AIB CvP firmware is not capable of handling a
data stream smaller than 4096bytes. The firmware's limitation
causes a hung-up as it's DMA engine waits forever for the
completion of the instructed 4096bytes.
To resolve this design limitation, both firmware and CvP
driver made several changes. At the CvP driver, we just
have to ensure that anything lesser than 4096bytes are
padded with extra bytes. The CvP will then, initiate the
tear-down by clearing the START_XFER and CVP_CONFIG bits.
We should also check for CVP_ERROR during the CvP completion.
A send_buf which is always 4096bytes is used to copy the
data during every transaction.

Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
---
changelog v2:
* Alignment fix parameter 'conf' altera_cvp_send_block
---
 drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index 4ffb9da537d8..5169f9bcd726 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -81,6 +81,7 @@ struct altera_cvp_conf {
 	u8			numclks;
 	u32			sent_packets;
 	u32			vsec_offset;
+	u8			*send_buf;
 	const struct cvp_priv	*priv;
 };
 
@@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
 		}
 
 		len = min(conf->priv->block_size, remaining);
-		altera_cvp_send_block(conf, data, len);
+		/* Copy the requested host data into the transmit buffer */
+
+		memcpy(conf->send_buf, data, len);
+		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
+				      conf->priv->block_size);
 		data += len / sizeof(u32);
 		done += len;
 		remaining -= len;
@@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
 	if (ret)
 		return ret;
 
-	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
-	altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val);
-	if (val & VSE_UNCOR_ERR_CVP_CFG_ERR) {
-		dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n");
+	/*
+	 * STEP 16 - If bitstream error (truncated/miss-matched),
+	 * we shall exit here.
+	 */
+	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
+	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
+		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");
 		return -EPROTO;
 	}
 
@@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 
 	pci_set_drvdata(pdev, mgr);
 
+	/* Allocate the 4096 block size transmit buffer */
+	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);
+	if (!conf->send_buf) {
+		ret = -ENOMEM;
+		goto err_unmap;
+	}
 	return 0;
 
 err_unmap:
-- 
2.25.1

Re: [PATCH v2] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Fri, May 20, 2022 at 09:30:40AM +0800, tien.sung.ang@intel.com wrote:
> From: Ang Tien Sung <tien.sung.ang@intel.com>
> 
> To support the error handling of a truncated bitstream sent.

A blank line here.

> The current AIB CvP firmware is not capable of handling a
> data stream smaller than 4096bytes. The firmware's limitation

So why don't you check the image size on write_init(), and just prevent
the DMA writing at the very beginning?

> causes a hung-up as it's DMA engine waits forever for the
> completion of the instructed 4096bytes.

A blank line here.

> To resolve this design limitation, both firmware and CvP
> driver made several changes. At the CvP driver, we just
> have to ensure that anything lesser than 4096bytes are
> padded with extra bytes. The CvP will then, initiate the
> tear-down by clearing the START_XFER and CVP_CONFIG bits.

The driver pads the data block to 4096 bytes, then why the CvP still
should fail the reprograming?

If the image size is larger than 1 Page but is not aligned to 1 Page,
will the reprogramming still fail?

> We should also check for CVP_ERROR during the CvP completion.
> A send_buf which is always 4096bytes is used to copy the
> data during every transaction.
> 
> Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> ---
> changelog v2:
> * Alignment fix parameter 'conf' altera_cvp_send_block
> ---
>  drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 4ffb9da537d8..5169f9bcd726 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -81,6 +81,7 @@ struct altera_cvp_conf {
>  	u8			numclks;
>  	u32			sent_packets;
>  	u32			vsec_offset;
> +	u8			*send_buf;
>  	const struct cvp_priv	*priv;
>  };
>  
> @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
>  		}
>  
>  		len = min(conf->priv->block_size, remaining);
> -		altera_cvp_send_block(conf, data, len);
> +		/* Copy the requested host data into the transmit buffer */
> +

This blank line is not needed.

> +		memcpy(conf->send_buf, data, len);

Any padding value is OK?

> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> +				      conf->priv->block_size);

If the len equals block_size, is the copy still needed?

>  		data += len / sizeof(u32);
>  		done += len;
>  		remaining -= len;
> @@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
>  	if (ret)
>  		return ret;
>  
> -	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
> -	altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val);
> -	if (val & VSE_UNCOR_ERR_CVP_CFG_ERR) {
> -		dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n");
> +	/*
> +	 * STEP 16 - If bitstream error (truncated/miss-matched),
> +	 * we shall exit here.
> +	 */
> +	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
> +	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
> +		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");

So this new error checking covers the previous "latched error" case?

>  		return -EPROTO;
>  	}
>  
> @@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev,
>  
>  	pci_set_drvdata(pdev, mgr);
>  
> +	/* Allocate the 4096 block size transmit buffer */
> +	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);

If block_size == ALTERA_CVP_V1_SIZE, the copy is still needed?

> +	if (!conf->send_buf) {
> +		ret = -ENOMEM;
> +		goto err_unmap;
> +	}

Maybe it is better move the buffer allocation to write_init()

Thanks,
Yilun

>  	return 0;
>  
>  err_unmap:
> -- 
> 2.25.1

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Thu, May 19, 2022 at 12:34:12PM +0800, tien.sung.ang@intel.com wrote:
> The send_buf is always used throughout the life-span of the CvP driver.
> Hence, we thought it would be wise to just pre-allocate it one time
> at the start of the probe/init.
> It is also fine if we do it in the altera_cvp_write. The only issue we
> see in this is that, a minor hit on the performance as you need to 
> then, allocate the buffer on every new CvP FPGA configuration write.
> 
> As for STEP 16, the previous implementation checks the Error latch bit
> which stores the previous transaction's result. If an error occurs
> prior to this, the driver would throw an error which is not right.
> The correct step is to just check for the current CvP error status 
> from the register.
> Hope that is fine with you. Thanks

Please comment inline in the mail, like others do.

Thanks,
Yilun

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Wed, May 18, 2022 at 01:08:31PM -0700, Tom Rix wrote:
> 
> On 5/17/22 11:48 PM, tien.sung.ang@intel.com wrote:
> > From: Ang Tien Sung <tien.sung.ang@intel.com>
> > 
> > To support the error handling of a truncated bitstream sent.
> > The current AIB CvP firmware is not capable of handling a
> > data stream smaller than 4096bytes. The firmware's limitation
> > causes a hung-up as it's DMA engine waits forever for the
> > completion of the instructed 4096bytes.
> > To resolve this design limitation, both firmware and CvP
> > driver made several changes. At the CvP driver, we just
> > have to ensure that anything lesser than 4096bytes are
> > padded with extra bytes. The CvP will then, initiate the
> > tear-down by clearing the START_XFER and CVP_CONFIG bits.
> > We should also check for CVP_ERROR during the CvP completion.
> > A send_buf which is always 4096bytes is used to copy the
> > data during every transaction.
> > 
> > Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
> > ---
> >   drivers/fpga/altera-cvp.c | 24 +++++++++++++++++++-----
> >   1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> > index 4ffb9da537d8..80edcfb5e5fc 100644
> > --- a/drivers/fpga/altera-cvp.c
> > +++ b/drivers/fpga/altera-cvp.c
> > @@ -81,6 +81,7 @@ struct altera_cvp_conf {
> >   	u8			numclks;
> >   	u32			sent_packets;
> >   	u32			vsec_offset;
> > +	u8			*send_buf;
> 
> Why is it necessary to hold  the send_buf in this structure ?
> 
> If it is used only in *_write, could it alloc/freed there ?
> 
> Because the write happens rarely, my preference is to alloc/free in
> *_write().

Is it better alloc in write_init()?

Thanks,
Yilun

> 
> >   	const struct cvp_priv	*priv;
> >   };
> > @@ -453,7 +454,11 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
> >   		}
> >   		len = min(conf->priv->block_size, remaining);
> > -		altera_cvp_send_block(conf, data, len);
> > +		/* Copy the requested host data into the transmit buffer */
> > +
> > +		memcpy(conf->send_buf, data, len);
> Is a memset needed for a short buffer ?
> > +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> > +		conf->priv->block_size);
> >   		data += len / sizeof(u32);
> >   		done += len;
> >   		remaining -= len;
> > @@ -492,10 +497,13 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
> >   	if (ret)
> >   		return ret;
> > -	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
> > -	altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val);
> > -	if (val & VSE_UNCOR_ERR_CVP_CFG_ERR) {
> > -		dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n");
> > +	/*
> > +	 * STEP 16 - If bitstream error (truncated/miss-matched),
> > +	 * we shall exit here.
> > +	 */
> > +	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
> Should this be STEP 17 ? the old 16 checked something else.
> 
> Tom
> 
> > +	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
> > +		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");
> >   		return -EPROTO;
> >   	}
> > @@ -661,6 +669,12 @@ static int altera_cvp_probe(struct pci_dev *pdev,
> >   	pci_set_drvdata(pdev, mgr);
> > +	/* Allocate the 4096 block size transmit buffer */
> > +	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);
> > +	if (!conf->send_buf) {
> > +		ret = -ENOMEM;
> > +		goto err_unmap;
> > +	}
> >   	return 0;
> >   err_unmap:

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Thu, May 19, 2022 at 12:21:35PM +0800, tien.sung.ang@intel.com wrote:
> Thanks for pointing that out. It is always good to get all the alignments right.

Please run checkpatch --strict before sending, it helps find out most of
alignment issues.

Thanks,
Yilun

> I will add this to the next revision of the patch.

Re: [PATCH v2] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

hi,

I will fix the "blank line" issues. 

>> The current AIB CvP firmware is not capable of handling a
>> data stream smaller than 4096bytes. The firmware's limitation

>So why don't you check the image size on write_init(), and just prevent
>the DMA writing at the very beginning?
We try not to add any kind of logic in the CvP driver to read
or understand the correctness of the bitstream. In future, this
logic may change. The CvP driver to our best interest should
only be responsible to perform the transfer of the bitstream
and perform the respective handshake. We have plans to have
the CvP firmware on the other side be more intelligent to 
handle errors and inform the CvP driver on why a certain 
configuration session fails. We are in the process of making
the next generation CvP to close this gaps.

>> To resolve this design limitation, both firmware and CvP
>> driver made several changes. At the CvP driver, we just
>> have to ensure that anything lesser than 4096bytes are
>> padded with extra bytes. The CvP will then, initiate the
>> tear-down by clearing the START_XFER and CVP_CONFIG bits.

> The driver pads the data block to 4096 bytes, then why the CvP still
> should fail the reprograming?
The block size of 4096bytes containts headers and signatures that
comprise a bitstream to FPGA. Any of the missing pieces will 
cause the FPGA configuration to fail at the SDM (Secure device
manager). We are at Intel are just trying to avoid a user from
using a bad data to reconfigure the FPGA via CvP and then, hitting
into a fault that is non-recoverable. Only a hard-reboot of the 
entire system can undo it. 

> If the image size is larger than 1 Page but is not aligned to 1 Page,
> will the reprogramming still fail?
Yes, the reconfiguration will fail. The above tear-down  is to prevent
that CvP Hardware/firmware in the FPGA from entering into a dead-lock.


>> +		memcpy(conf->send_buf, data, len);

>Any padding value is OK?
Yes, any padding value is fine. The reason for this is that, the
CvP Firmware/Hardware on the other side is using DMA to perform
transfers of 4096bytes. The firmware will wait forever for the
DMA to complete. The firmware is not able to check for status
or respond to any errors while in the wait state. 

>> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
>> +				      conf->priv->block_size);

>If the len equals block_size, is the copy still needed?
Actually, not required. But to maintain a simple design, we use
a common flow for all so you can skip the check.

>> +	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
>> +	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
>> +		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");

>So this new error checking covers the previous "latched error" case?
Yes, because the new feature requires for the CvP driver to 
be able to subsequently re-perform a fresh new CvP session after
an error. With the above, the CvP driver will forever report
an error and caused the entire CvP to be permanently in error. 
This is not desirable and the user would need to power-cycle the
or hard reboot the entire system to recover. Intel sees this as
a terrible user-experience espcially as CvP is performed 
normally remotely.

>> +	/* Allocate the 4096 block size transmit buffer */
>> +	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);

>If block_size == ALTERA_CVP_V1_SIZE, the copy is still needed?
Yes, the firmware and hardware on the other side uses DMA
and V1 to my understanding has a smaller buffer size. Intel 
later improved this and introduced a new back-pressure credit 
data flow control to help increase the performance.

>> +	if (!conf->send_buf) {
>> +		ret = -ENOMEM;
>> +		goto err_unmap;
>> +	}

>Maybe it is better move the buffer allocation to write_init()
The altera_cvp_conf has not been created yet at this point.
The init routine in CvP so far just registers the driver.

Re: [PATCH v2] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Tue, May 31, 2022 at 09:53:56AM +0800, tien.sung.ang@intel.com wrote:
> hi,
> 
> I will fix the "blank line" issues. 

Please don't top post.

And you don't have to clean too much context, otherwise people have to
search previous mails to remember what is it. People may not read the
mail at once.

> 
> >> The current AIB CvP firmware is not capable of handling a
> >> data stream smaller than 4096bytes. The firmware's limitation
> 
> >So why don't you check the image size on write_init(), and just prevent
> >the DMA writing at the very beginning?
> We try not to add any kind of logic in the CvP driver to read
> or understand the correctness of the bitstream. In future, this
> logic may change. The CvP driver to our best interest should

But you are adding the logic in altera_cvp_write() to keep the
correctness. What's the difference adding it somewhere else? And as I
can see, preventing the writing at the very beginning is a much cleaner
way.

> only be responsible to perform the transfer of the bitstream
> and perform the respective handshake. We have plans to have
> the CvP firmware on the other side be more intelligent to 
> handle errors and inform the CvP driver on why a certain 
> configuration session fails. We are in the process of making
> the next generation CvP to close this gaps.
> 
> >> To resolve this design limitation, both firmware and CvP
> >> driver made several changes. At the CvP driver, we just
> >> have to ensure that anything lesser than 4096bytes are
> >> padded with extra bytes. The CvP will then, initiate the
> >> tear-down by clearing the START_XFER and CVP_CONFIG bits.
> 
> > The driver pads the data block to 4096 bytes, then why the CvP still
> > should fail the reprograming?
> The block size of 4096bytes containts headers and signatures that
> comprise a bitstream to FPGA. Any of the missing pieces will 
> cause the FPGA configuration to fail at the SDM (Secure device
> manager). We are at Intel are just trying to avoid a user from
> using a bad data to reconfigure the FPGA via CvP and then, hitting
> into a fault that is non-recoverable. Only a hard-reboot of the 
> entire system can undo it. 
> 
> > If the image size is larger than 1 Page but is not aligned to 1 Page,
> > will the reprogramming still fail?
> Yes, the reconfiguration will fail. The above tear-down  is to prevent
> that CvP Hardware/firmware in the FPGA from entering into a dead-lock.

So if the image size is not aligned to 1 Page, the reprogram should fail
anyway, is it? Then I really recommend you fail the reprogramming at the
very beginning.

> 
> 
> >> +		memcpy(conf->send_buf, data, len);
> 
> >Any padding value is OK?
> Yes, any padding value is fine. The reason for this is that, the
> CvP Firmware/Hardware on the other side is using DMA to perform
> transfers of 4096bytes. The firmware will wait forever for the
> DMA to complete. The firmware is not able to check for status
> or respond to any errors while in the wait state. 
> 
> >> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> >> +				      conf->priv->block_size);
> 
> >If the len equals block_size, is the copy still needed?
> Actually, not required. But to maintain a simple design, we use
> a common flow for all so you can skip the check.

I don't think so. We should make the code reasonable. Blindly copy the
whole buffer impacts all normal cases, and makes people confused. The
code seems shorter now, but not simpler, it takes people more time to
figure out why.

> 
> >> +	ret = altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
> >> +	if (ret || (val & VSE_CVP_STATUS_CFG_ERR)) {
> >> +		dev_err(&mgr->dev, "CVP_CONFIG_ERROR!\n");
> 
> >So this new error checking covers the previous "latched error" case?
> Yes, because the new feature requires for the CvP driver to 
> be able to subsequently re-perform a fresh new CvP session after
> an error. With the above, the CvP driver will forever report
> an error and caused the entire CvP to be permanently in error. 
> This is not desirable and the user would need to power-cycle the
> or hard reboot the entire system to recover. Intel sees this as
> a terrible user-experience espcially as CvP is performed 
> normally remotely.
> 
> >> +	/* Allocate the 4096 block size transmit buffer */
> >> +	conf->send_buf = devm_kzalloc(&pdev->dev, conf->priv->block_size, GFP_KERNEL);
> 
> >If block_size == ALTERA_CVP_V1_SIZE, the copy is still needed?
> Yes, the firmware and hardware on the other side uses DMA
> and V1 to my understanding has a smaller buffer size. Intel 
> later improved this and introduced a new back-pressure credit 
> data flow control to help increase the performance.
> 
> >> +	if (!conf->send_buf) {
> >> +		ret = -ENOMEM;
> >> +		goto err_unmap;
> >> +	}
> 
> >Maybe it is better move the buffer allocation to write_init()
> The altera_cvp_conf has not been created yet at this point.
> The init routine in CvP so far just registers the driver.

I meant moving it to altera_cvp_write_init(). But now I think it is not
necessary. Bouncing buffer is not necessary at all.

Thanks,
Yilun

Re: [PATCH v2] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

> But you are adding the logic in altera_cvp_write() to keep the
> correctness. What's the difference adding it somewhere else? And as I
> can see, preventing the writing at the very beginning is a much cleaner
> way.

Though without doubt, your solution is doable, I have a 
discussion with the Intel architect and they insisted that
the device driver must not make such a decision. The decision to 
drop or accept a transfer is up to the firmware. The firmware
insisted that the buffer be padded with whatever value. They
desire the transfer protocol to be obeyed to ensure, that 
there is no hard dependencies on the device driver if they
do one day change the 4kbytes to some other smaller value.

> > > If the image size is larger than 1 Page but is not aligned to 1 Page,
> > > will the reprogramming still fail?
> > Yes, the reconfiguration will fail. The above tear-down  is to prevent
> > that CvP Hardware/firmware in the FPGA from entering into a dead-lock.

> So if the image size is not aligned to 1 Page, the reprogram should fail
> anyway, is it? Then I really recommend you fail the reprogramming at the
> very beginning.

Same reasoning as above. We don't want the driver to make this
decision.

> > >> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> > >> +				      conf->priv->block_size);
> > 
> > >If the len equals block_size, is the copy still needed?
> > Actually, not required. But to maintain a simple design, we use
> > a common flow for all so you can skip the check.

> I don't think so. We should make the code reasonable. Blindly copy the
> whole buffer impacts all normal cases, and makes people confused. The
> code seems shorter now, but not simpler, it takes people more time to
> figure out why.

Yes, it could look confusing to other programmers. And, yes, the 
padding doesn't matter. Let me relook into this. As the driver is 
already re-tested by the silicon validatioin. 
I want to avoid making any change as it
would meant another couple of weeks of re-testing. 
Can this be accepted as it is?

Re: [PATCH v2] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Tue, Jun 07, 2022 at 01:55:30PM +0800, tien.sung.ang@intel.com wrote:
> > But you are adding the logic in altera_cvp_write() to keep the
> > correctness. What's the difference adding it somewhere else? And as I
> > can see, preventing the writing at the very beginning is a much cleaner
> > way.
> 
> Though without doubt, your solution is doable, I have a 
> discussion with the Intel architect and they insisted that
> the device driver must not make such a decision. The decision to 
> drop or accept a transfer is up to the firmware. The firmware

Why dropping or accepting is forbidden, but padding is allowed? If the
decision is no operation to the data, then padding should also be
forbidden.

> insisted that the buffer be padded with whatever value. They

If I understand right, the 2 decisions are conflicted. On one hand,
the driver should not care about the data. On another hand, the
driver should still care about the data for some rule.

So please firstly reach your agreement before making patches.

> desire the transfer protocol to be obeyed to ensure, that 
> there is no hard dependencies on the device driver if they
> do one day change the 4kbytes to some other smaller value.
> 
> > > > If the image size is larger than 1 Page but is not aligned to 1 Page,
> > > > will the reprogramming still fail?
> > > Yes, the reconfiguration will fail. The above tear-down  is to prevent
> > > that CvP Hardware/firmware in the FPGA from entering into a dead-lock.
> 
> > So if the image size is not aligned to 1 Page, the reprogram should fail
> > anyway, is it? Then I really recommend you fail the reprogramming at the
> > very beginning.
> 
> Same reasoning as above. We don't want the driver to make this
> decision.
> 
> > > >> +		altera_cvp_send_block(conf, (const u32 *)conf->send_buf,
> > > >> +				      conf->priv->block_size);
> > > 
> > > >If the len equals block_size, is the copy still needed?
> > > Actually, not required. But to maintain a simple design, we use
> > > a common flow for all so you can skip the check.
> 
> > I don't think so. We should make the code reasonable. Blindly copy the
> > whole buffer impacts all normal cases, and makes people confused. The
> > code seems shorter now, but not simpler, it takes people more time to
> > figure out why.
> 
> Yes, it could look confusing to other programmers. And, yes, the 
> padding doesn't matter. Let me relook into this. As the driver is 
> already re-tested by the silicon validatioin. 
> I want to avoid making any change as it
> would meant another couple of weeks of re-testing. 
> Can this be accepted as it is?

Sorry, I can't. Some clarification is needed.

Thanks,
Yilun

Re: [PATCH v2] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

> > Though without doubt, your solution is doable, I have a 
> > discussion with the Intel architect and they insisted that
> > the device driver must not make such a decision. The decision to 
> > drop or accept a transfer is up to the firmware. The firmware

> Why dropping or accepting is forbidden, but padding is allowed? If the
> decision is no operation to the data, then padding should also be
> forbidden.

> > insisted that the buffer be padded with whatever value. They

> If I understand right, the 2 decisions are conflicted. On one hand,
> the driver should not care about the data. On another hand, the
> driver should still care about the data for some rule.

> So please firstly reach your agreement before making patches.

Had some delays in replying,from the perspective of the driver, 
Intel architects want it to be just purely on the transfer of 
data without inspecting the data. 
If the size of a transfer is 4096bytes, we can padd it. 
I can do that which I think it is not an issue.
Let me create a reworked patch to pad the payload with Zeros.
Would this be OK for patch v3?

> > Yes, it could look confusing to other programmers. And, yes, the 
> > padding doesn't matter. Let me relook into this. As the driver is 
> > already re-tested by the silicon validatioin. 
> > I want to avoid making any change as it
> > would meant another couple of weeks of re-testing. 
> > Can this be accepted as it is?

> Sorry, I can't. Some clarification is needed.

Reply as above, let us implement the padding in the
next patch v3. Appreciate your inputs. Thanks

Re: [PATCH v2] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Mon, Jun 27, 2022 at 03:45:53PM +0800, tien.sung.ang@intel.com wrote:
> > > Though without doubt, your solution is doable, I have a 
> > > discussion with the Intel architect and they insisted that
> > > the device driver must not make such a decision. The decision to 
> > > drop or accept a transfer is up to the firmware. The firmware
> 
> > Why dropping or accepting is forbidden, but padding is allowed? If the
> > decision is no operation to the data, then padding should also be
> > forbidden.
> 
> > > insisted that the buffer be padded with whatever value. They
> 
> > If I understand right, the 2 decisions are conflicted. On one hand,
> > the driver should not care about the data. On another hand, the
> > driver should still care about the data for some rule.
> 
> > So please firstly reach your agreement before making patches.
> 
> Had some delays in replying,from the perspective of the driver, 
> Intel architects want it to be just purely on the transfer of 
> data without inspecting the data. 
> If the size of a transfer is 4096bytes, we can padd it. 
> I can do that which I think it is not an issue.
> Let me create a reworked patch to pad the payload with Zeros.

No, it's not about padding with Zeros or other values. If we should
stick on no image touching, then please fix the firmware. If we need
a workaround in driver, then making a decent fix. Blindly copy the image
is just a bad idea, and it could actually be avoided.

Thanks,
Yilun

> Would this be OK for patch v3?
> 
> > > Yes, it could look confusing to other programmers. And, yes, the 
> > > padding doesn't matter. Let me relook into this. As the driver is 
> > > already re-tested by the silicon validatioin. 
> > > I want to avoid making any change as it
> > > would meant another couple of weeks of re-testing. 
> > > Can this be accepted as it is?
> 
> > Sorry, I can't. Some clarification is needed.
> 
> Reply as above, let us implement the padding in the
> next patch v3. Appreciate your inputs. Thanks

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[Xu Yilun]

On Thu, May 19, 2022 at 05:39:07PM +0800, tien.sung.ang@intel.com wrote:
> Thanks for bringing this up. Yes, you are right that the fpga_mgr sees this
> as an error irrespective of the value. The CvP driver is changed now to just
> indicate the correct error which recommends a retry. To me understanding,
> EAGAIN was this. The fpga manager now looks like is going to return a CvP
> failure in short. 
> A BUSY state does not seem to be able to solve this issue. 
> Even an extended time-out didn't resolve this error state. The current time-out
> is set to 10seconds.   
> However, the main objective is to also handle the error if the CvP firmware
> is not responsive. The error_path flow is to reset the CVP mode and HIP_CLK_SEL bit

Please add your main objective to commit message.

Thanks,
Yilun

> as recommended by the firmware engineers. 
> The flow prescribed here is also an identical copy of working CvP driver 
> which is also owned by Intel. This driver is a downstream driver which is 
> not part of the Linux kernel. We are now porting this differences over to 
> the current upstream CvP driver. 

Re: [PATCH] fpga: altera-cvp: Truncated bitstream error support

[tien.sung.ang]

Thanks for bringing this up. Yes, you are right that the fpga_mgr sees this
as an error irrespective of the value. The CvP driver is changed now to just
indicate the correct error which recommends a retry. To me understanding,
EAGAIN was this. The fpga manager now looks like is going to return a CvP
failure in short. 
A BUSY state does not seem to be able to solve this issue. 
Even an extended time-out didn't resolve this error state. The current time-out
is set to 10seconds.   
However, the main objective is to also handle the error if the CvP firmware
is not responsive. The error_path flow is to reset the CVP mode and HIP_CLK_SEL bit
as recommended by the firmware engineers. 
The flow prescribed here is also an identical copy of working CvP driver 
which is also owned by Intel. This driver is a downstream driver which is 
not part of the Linux kernel. We are now porting this differences over to 
the current upstream CvP driver.