We have some machines with ASPEED SATA controllers, and are seeing the same NCQ
issues that ATI controllers (I am not sure if it's a rebranded ATI controller,
or they both have some faulty implementation). This NCQ breakage is consistent
across a few different types of drives.
Instead of maintaining a list of drives that are broken with ASPEED controllers
as well as AIT, let's just treat ASPEED controllers like ATI ones, and disable
NCQ on drives that have ATA_HORKAGE_NO_NCQ_ON_ATI set on them.
To do this first, we have to make move the definition of the ASPEED vendor from
the ast drm driver to the PCI subsystem.
We have some machines with ASPEED SATA controllers, and are seeing the same NCQ
issues that ATI controllers (I am not sure if it's a rebranded ATI controller,
or they both have some faulty implementation). This NCQ breakage is consistent
across a few different types of drives.
Instead of maintaining a list of drives that are broken with ASPEED controllers
as well as ATI, let's just treat ASPEED controllers like ATI ones, and disable
NCQ on drives that have ATA_HORKAGE_NO_NCQ_ON_ATI set on them.
We have been running this patch on several machines for over a week now without
reproducing an issue that was happening almost daily before.
Signed-off-by: Patrick McLean <[email protected]>
---
drivers/ata/libata-core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 14c17c3bda4e..051492e8e9f9 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2219,7 +2219,8 @@ static int ata_dev_config_ncq(struct ata_device *dev,
}
if (dev->horkage & ATA_HORKAGE_NO_NCQ_ON_ATI &&
- ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) {
+ (ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI) ||
+ ata_dev_check_adapter(dev, PCI_VENDOR_ID_ASPEED))) {
snprintf(desc, desc_sz, "NCQ (not used)");
return 0;
}
--
2.40.0
Currently the ASPEED PCI vendor ID is defined in drivers/gpu/drm/ast/ast_drv.c,
move that to include/linux/pci_ids.h with all the rest of the PCI vendor ID
definitions. Rename the definition to follow the format that the other
definitions follow.
Signed-off-by: Patrick McLean <[email protected]>
---
drivers/gpu/drm/ast/ast_drv.c | 4 +---
include/linux/pci_ids.h | 2 ++
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index d78852c7cf5b..232e797793b6 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -70,12 +70,10 @@ static const struct drm_driver ast_driver = {
* PCI driver
*/
-#define PCI_VENDOR_ASPEED 0x1a03
-
#define AST_VGA_DEVICE(id, info) { \
.class = PCI_BASE_CLASS_DISPLAY << 16, \
.class_mask = 0xff0000, \
- .vendor = PCI_VENDOR_ASPEED, \
+ .vendor = PCI_VENDOR_ID_ASPEED, \
.device = id, \
.subvendor = PCI_ANY_ID, \
.subdevice = PCI_ANY_ID, \
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 45c3d62e616d..6634741aea80 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -815,6 +815,8 @@
#define PCI_VENDOR_ID_ASUSTEK 0x1043
#define PCI_DEVICE_ID_ASUSTEK_0675 0x0675
+#define PCI_VENDOR_ID_ASPEED 0x1a03
+
#define PCI_VENDOR_ID_DPT 0x1044
#define PCI_DEVICE_ID_DPT 0xa400
--
2.40.0
On Mon, Apr 17, 2023 at 06:17:20PM -0700, Patrick McLean wrote:
> We have some machines with ASPEED SATA controllers, and are seeing the same NCQ
> issues that ATI controllers (I am not sure if it's a rebranded ATI controller,
> or they both have some faulty implementation). This NCQ breakage is consistent
> across a few different types of drives.
>
> Instead of maintaining a list of drives that are broken with ASPEED controllers
Are these ASPEED controllers all the same or a wide variety?
Quirking all controllers from the same vendor seems like an overly
broad approach to me.
On 4/18/23 14:24, Christoph Hellwig wrote:
> On Mon, Apr 17, 2023 at 06:17:20PM -0700, Patrick McLean wrote:
>> We have some machines with ASPEED SATA controllers, and are seeing the same NCQ
>> issues that ATI controllers (I am not sure if it's a rebranded ATI controller,
>> or they both have some faulty implementation). This NCQ breakage is consistent
>> across a few different types of drives.
>>
>> Instead of maintaining a list of drives that are broken with ASPEED controllers
>
> Are these ASPEED controllers all the same or a wide variety?
> Quirking all controllers from the same vendor seems like an overly
> broad approach to me.
Indeed. If you checked only one adapter model from ASPEED, then all that is
needed is define it with "board_ahci_noncq" in drivers/ata/ahci.c (see
ahci_pci_tbl array). NCQ support will be turned off for that particular adapter
with that.
On Mon, Apr 17, 2023 at 06:17:19PM -0700, Patrick McLean wrote:
> Currently the ASPEED PCI vendor ID is defined in drivers/gpu/drm/ast/ast_drv.c,
> move that to include/linux/pci_ids.h with all the rest of the PCI vendor ID
> definitions. Rename the definition to follow the format that the other
> definitions follow.
>
> Signed-off-by: Patrick McLean <[email protected]>
Acked-by: Daniel Vetter <[email protected]>
for merging through whatever tree this series lands through.
-Daniel
> ---
> drivers/gpu/drm/ast/ast_drv.c | 4 +---
> include/linux/pci_ids.h | 2 ++
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index d78852c7cf5b..232e797793b6 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -70,12 +70,10 @@ static const struct drm_driver ast_driver = {
> * PCI driver
> */
>
> -#define PCI_VENDOR_ASPEED 0x1a03
> -
> #define AST_VGA_DEVICE(id, info) { \
> .class = PCI_BASE_CLASS_DISPLAY << 16, \
> .class_mask = 0xff0000, \
> - .vendor = PCI_VENDOR_ASPEED, \
> + .vendor = PCI_VENDOR_ID_ASPEED, \
> .device = id, \
> .subvendor = PCI_ANY_ID, \
> .subdevice = PCI_ANY_ID, \
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 45c3d62e616d..6634741aea80 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -815,6 +815,8 @@
> #define PCI_VENDOR_ID_ASUSTEK 0x1043
> #define PCI_DEVICE_ID_ASUSTEK_0675 0x0675
>
> +#define PCI_VENDOR_ID_ASPEED 0x1a03
> +
> #define PCI_VENDOR_ID_DPT 0x1044
> #define PCI_DEVICE_ID_DPT 0xa400
>
> --
> 2.40.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hello!
On 4/18/23 4:17 AM, Patrick McLean wrote:
> We have some machines with ASPEED SATA controllers, and are seeing the same NCQ
> issues that ATI controllers (I am not sure if it's a rebranded ATI controller,
> or they both have some faulty implementation). This NCQ breakage is consistent
> across a few different types of drives.
>
> Instead of maintaining a list of drives that are broken with ASPEED controllers
> as well as ATI, let's just treat ASPEED controllers like ATI ones, and disable
> NCQ on drives that have ATA_HORKAGE_NO_NCQ_ON_ATI set on them.
>
> We have been running this patch on several machines for over a week now without
> reproducing an issue that was happening almost daily before.
>
> Signed-off-by: Patrick McLean <[email protected]>
> ---
> drivers/ata/libata-core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 14c17c3bda4e..051492e8e9f9 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2219,7 +2219,8 @@ static int ata_dev_config_ncq(struct ata_device *dev,
> }
>
> if (dev->horkage & ATA_HORKAGE_NO_NCQ_ON_ATI &&
> - ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) {
> + (ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI) ||
> + ata_dev_check_adapter(dev, PCI_VENDOR_ID_ASPEED))) {
Please align the start of this line with the start of the above
line, so that it doesn't needlessly blend with the below line.
> snprintf(desc, desc_sz, "NCQ (not used)");
> return 0;
> }
MBR, Sergey
On 4/18/23 4:17 AM, Patrick McLean wrote:
> Currently the ASPEED PCI vendor ID is defined in drivers/gpu/drm/ast/ast_drv.c,
> move that to include/linux/pci_ids.h with all the rest of the PCI vendor ID
> definitions. Rename the definition to follow the format that the other
> definitions follow.
>
> Signed-off-by: Patrick McLean <[email protected]>
[...]
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 45c3d62e616d..6634741aea80 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -815,6 +815,8 @@
> #define PCI_VENDOR_ID_ASUSTEK 0x1043
> #define PCI_DEVICE_ID_ASUSTEK_0675 0x0675
>
> +#define PCI_VENDOR_ID_ASPEED 0x1a03
> +
> #define PCI_VENDOR_ID_DPT 0x1044
> #define PCI_DEVICE_ID_DPT 0xa400
>
The vendor IDs in this file are sorted numerically, not
alphabetically...
MBR, Sergey
Most subject lines for pci_ids.h look like this:
PCI: Add ASPEED vendor ID
On Mon, Apr 17, 2023 at 06:17:19PM -0700, Patrick McLean wrote:
> Currently the ASPEED PCI vendor ID is defined in drivers/gpu/drm/ast/ast_drv.c,
> move that to include/linux/pci_ids.h with all the rest of the PCI vendor ID
> definitions. Rename the definition to follow the format that the other
> definitions follow.
>
> Signed-off-by: Patrick McLean <[email protected]>
Given the subject line and file placement (below) updates,
Acked-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/gpu/drm/ast/ast_drv.c | 4 +---
> include/linux/pci_ids.h | 2 ++
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index d78852c7cf5b..232e797793b6 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -70,12 +70,10 @@ static const struct drm_driver ast_driver = {
> * PCI driver
> */
>
> -#define PCI_VENDOR_ASPEED 0x1a03
> -
> #define AST_VGA_DEVICE(id, info) { \
> .class = PCI_BASE_CLASS_DISPLAY << 16, \
> .class_mask = 0xff0000, \
> - .vendor = PCI_VENDOR_ASPEED, \
> + .vendor = PCI_VENDOR_ID_ASPEED, \
> .device = id, \
> .subvendor = PCI_ANY_ID, \
> .subdevice = PCI_ANY_ID, \
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 45c3d62e616d..6634741aea80 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -815,6 +815,8 @@
> #define PCI_VENDOR_ID_ASUSTEK 0x1043
> #define PCI_DEVICE_ID_ASUSTEK_0675 0x0675
>
> +#define PCI_VENDOR_ID_ASPEED 0x1a03
This looks like a random placement. Please keep sorted by numeric
vendor ID. I'll make the comment at the top a little more specific.
> #define PCI_VENDOR_ID_DPT 0x1044
> #define PCI_DEVICE_ID_DPT 0xa400
>
> --
> 2.40.0
>
On Tue, Apr 18, 2023 at 04:14:03PM -0500, Bjorn Helgaas wrote:
> Most subject lines for pci_ids.h look like this:
>
> PCI: Add ASPEED vendor ID
>
> On Mon, Apr 17, 2023 at 06:17:19PM -0700, Patrick McLean wrote:
> > Currently the ASPEED PCI vendor ID is defined in drivers/gpu/drm/ast/ast_drv.c,
> > move that to include/linux/pci_ids.h with all the rest of the PCI vendor ID
> > definitions. Rename the definition to follow the format that the other
> > definitions follow.
> >
> > Signed-off-by: Patrick McLean <[email protected]>
>
> Given the subject line and file placement (below) updates,
>
> Acked-by: Bjorn Helgaas <[email protected]>
Oh, at the same time, would you mind rewrapping at least this commit
log so it fits in 75 columns to "git log" doesn't overflow an 80
column terminal?
Bjorn