diff mbox series

[v2,1/1,SRU,F,OEM-B-OSP1] thunderbolt: Retry DROM read once if parsing fails

Message ID 20200918055621.254184-2-acelan.kao@canonical.com
State New
Headers show
Series Thunderbolt3 daisy chain sometimes doesn't work Edit | expand

Commit Message

AceLan Kao Sept. 18, 2020, 5:56 a.m. UTC 
From: Mika Westerberg <mika.westerberg@linux.intel.com>

BugLink: https://bugs.launchpad.net/bugs/1895606

Kai-Heng reported that sometimes DROM parsing of ASUS PA27AC Thunderbolt 3
monitor fails. This makes the driver to fail to add the device so only
DisplayPort tunneling is functional.

It is not clear what exactly happens but waiting for 100 ms and retrying
the read seems to work this around so we do that here.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=206493
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
(bacported from commit f022ff7bf377ca94367be05de61277934d42ea74
linux-next)
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/thunderbolt/eeprom.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Colin Ian King Sept. 18, 2020, 8:56 a.m. UTC | #1 
On 18/09/2020 06:56, AceLan Kao wrote:
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1895606
> 
> Kai-Heng reported that sometimes DROM parsing of ASUS PA27AC Thunderbolt 3
> monitor fails. This makes the driver to fail to add the device so only
> DisplayPort tunneling is functional.
> 
> It is not clear what exactly happens but waiting for 100 ms and retrying
> the read seems to work this around so we do that here.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206493
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> (bacported from commit f022ff7bf377ca94367be05de61277934d42ea74
> linux-next)
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> ---
>  drivers/thunderbolt/eeprom.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index ee5196479854..3f7baf5aa524 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/crc32.h>
> +#include <linux/delay.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  #include "tb.h"
> @@ -386,8 +387,8 @@ static int tb_drom_parse_entries(struct tb_switch *sw)
>  		struct tb_drom_entry_header *entry = (void *) (sw->drom + pos);
>  		if (pos + 1 == drom_size || pos + entry->len > drom_size
>  				|| !entry->len) {
> -			tb_sw_warn(sw, "drom buffer overrun, aborting\n");
> -			return -EIO;
> +			tb_sw_warn(sw, "DROM buffer overrun\n");
> +			return -EILSEQ;
>  		}
>  
>  		switch (entry->type) {
> @@ -493,7 +494,8 @@ int tb_drom_read(struct tb_switch *sw)
>  	u16 size;
>  	u32 crc;
>  	struct tb_drom_header *header;
> -	int res;
> +	int res, retries = 1;
> +
>  	if (sw->drom)
>  		return 0;
>  
> @@ -581,7 +583,17 @@ int tb_drom_read(struct tb_switch *sw)
>  		tb_sw_warn(sw, "drom device_rom_revision %#x unknown\n",
>  			header->device_rom_revision);
>  
> -	return tb_drom_parse_entries(sw);
> +	res = tb_drom_parse_entries(sw);
> +	/* If the DROM parsing fails, wait a moment and retry once */
> +	if (res == -EILSEQ && retries--) {
> +		tb_sw_warn(sw, "parsing DROM failed, retrying\n");
> +		msleep(100);
> +		res = tb_eeprom_read_n(sw, 0, sw->drom, size);
> +		if (!res)
> +			goto parse;
> +	}
> +
> +	return res;
>  err:
>  	kfree(sw->drom);
>  	sw->drom = NULL;
> 

This looks like a suitable hack^H^H^H^H workaround. I think this will be
useful for Groovy too.

Acked-by: Colin Ian King <colin.king@canonical.com>
Stefan Bader Sept. 21, 2020, 7:56 a.m. UTC | #2 
On 18.09.20 07:56, AceLan Kao wrote:
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1895606
> 
> Kai-Heng reported that sometimes DROM parsing of ASUS PA27AC Thunderbolt 3
> monitor fails. This makes the driver to fail to add the device so only
> DisplayPort tunneling is functional.
> 
> It is not clear what exactly happens but waiting for 100 ms and retrying
> the read seems to work this around so we do that here.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206493
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> (bacported from commit f022ff7bf377ca94367be05de61277934d42ea74
> linux-next)
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---

It looks something went wrong with this patch. Not sure whether this was about
the v1 but the bug report said committed but I checked and nothing was really
committed. So this is still pending and I changed the status back.

-Stefan


>  drivers/thunderbolt/eeprom.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index ee5196479854..3f7baf5aa524 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/crc32.h>
> +#include <linux/delay.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  #include "tb.h"
> @@ -386,8 +387,8 @@ static int tb_drom_parse_entries(struct tb_switch *sw)
>  		struct tb_drom_entry_header *entry = (void *) (sw->drom + pos);
>  		if (pos + 1 == drom_size || pos + entry->len > drom_size
>  				|| !entry->len) {
> -			tb_sw_warn(sw, "drom buffer overrun, aborting\n");
> -			return -EIO;
> +			tb_sw_warn(sw, "DROM buffer overrun\n");
> +			return -EILSEQ;
>  		}
>  
>  		switch (entry->type) {
> @@ -493,7 +494,8 @@ int tb_drom_read(struct tb_switch *sw)
>  	u16 size;
>  	u32 crc;
>  	struct tb_drom_header *header;
> -	int res;
> +	int res, retries = 1;
> +
>  	if (sw->drom)
>  		return 0;
>  
> @@ -581,7 +583,17 @@ int tb_drom_read(struct tb_switch *sw)
>  		tb_sw_warn(sw, "drom device_rom_revision %#x unknown\n",
>  			header->device_rom_revision);
>  
> -	return tb_drom_parse_entries(sw);
> +	res = tb_drom_parse_entries(sw);
> +	/* If the DROM parsing fails, wait a moment and retry once */
> +	if (res == -EILSEQ && retries--) {
> +		tb_sw_warn(sw, "parsing DROM failed, retrying\n");
> +		msleep(100);
> +		res = tb_eeprom_read_n(sw, 0, sw->drom, size);
> +		if (!res)
> +			goto parse;
> +	}
> +
> +	return res;
>  err:
>  	kfree(sw->drom);
>  	sw->drom = NULL;
>
Ian May Oct. 2, 2020, 10:21 p.m. UTC | #3 
Applied to Focal/master-next.  Thanks!

Ian

On 2020-09-18 13:56:21 , AceLan Kao wrote:
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1895606
> 
> Kai-Heng reported that sometimes DROM parsing of ASUS PA27AC Thunderbolt 3
> monitor fails. This makes the driver to fail to add the device so only
> DisplayPort tunneling is functional.
> 
> It is not clear what exactly happens but waiting for 100 ms and retrying
> the read seems to work this around so we do that here.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206493
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> (bacported from commit f022ff7bf377ca94367be05de61277934d42ea74
> linux-next)
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> ---
>  drivers/thunderbolt/eeprom.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index ee5196479854..3f7baf5aa524 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/crc32.h>
> +#include <linux/delay.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  #include "tb.h"
> @@ -386,8 +387,8 @@ static int tb_drom_parse_entries(struct tb_switch *sw)
>  		struct tb_drom_entry_header *entry = (void *) (sw->drom + pos);
>  		if (pos + 1 == drom_size || pos + entry->len > drom_size
>  				|| !entry->len) {
> -			tb_sw_warn(sw, "drom buffer overrun, aborting\n");
> -			return -EIO;
> +			tb_sw_warn(sw, "DROM buffer overrun\n");
> +			return -EILSEQ;
>  		}
>  
>  		switch (entry->type) {
> @@ -493,7 +494,8 @@ int tb_drom_read(struct tb_switch *sw)
>  	u16 size;
>  	u32 crc;
>  	struct tb_drom_header *header;
> -	int res;
> +	int res, retries = 1;
> +
>  	if (sw->drom)
>  		return 0;
>  
> @@ -581,7 +583,17 @@ int tb_drom_read(struct tb_switch *sw)
>  		tb_sw_warn(sw, "drom device_rom_revision %#x unknown\n",
>  			header->device_rom_revision);
>  
> -	return tb_drom_parse_entries(sw);
> +	res = tb_drom_parse_entries(sw);
> +	/* If the DROM parsing fails, wait a moment and retry once */
> +	if (res == -EILSEQ && retries--) {
> +		tb_sw_warn(sw, "parsing DROM failed, retrying\n");
> +		msleep(100);
> +		res = tb_eeprom_read_n(sw, 0, sw->drom, size);
> +		if (!res)
> +			goto parse;
> +	}
> +
> +	return res;
>  err:
>  	kfree(sw->drom);
>  	sw->drom = NULL;
> -- 
> 2.25.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff mbox series

Patch

diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index ee5196479854..3f7baf5aa524 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -7,6 +7,7 @@ 
  */
 
 #include <linux/crc32.h>
+#include <linux/delay.h>
 #include <linux/property.h>
 #include <linux/slab.h>
 #include "tb.h"
@@ -386,8 +387,8 @@  static int tb_drom_parse_entries(struct tb_switch *sw)
 		struct tb_drom_entry_header *entry = (void *) (sw->drom + pos);
 		if (pos + 1 == drom_size || pos + entry->len > drom_size
 				|| !entry->len) {
-			tb_sw_warn(sw, "drom buffer overrun, aborting\n");
-			return -EIO;
+			tb_sw_warn(sw, "DROM buffer overrun\n");
+			return -EILSEQ;
 		}
 
 		switch (entry->type) {
@@ -493,7 +494,8 @@  int tb_drom_read(struct tb_switch *sw)
 	u16 size;
 	u32 crc;
 	struct tb_drom_header *header;
-	int res;
+	int res, retries = 1;
+
 	if (sw->drom)
 		return 0;
 
@@ -581,7 +583,17 @@  int tb_drom_read(struct tb_switch *sw)
 		tb_sw_warn(sw, "drom device_rom_revision %#x unknown\n",
 			header->device_rom_revision);
 
-	return tb_drom_parse_entries(sw);
+	res = tb_drom_parse_entries(sw);
+	/* If the DROM parsing fails, wait a moment and retry once */
+	if (res == -EILSEQ && retries--) {
+		tb_sw_warn(sw, "parsing DROM failed, retrying\n");
+		msleep(100);
+		res = tb_eeprom_read_n(sw, 0, sw->drom, size);
+		if (!res)
+			goto parse;
+	}
+
+	return res;
 err:
 	kfree(sw->drom);
 	sw->drom = NULL;