| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <70853f13-f74e-d9bb-89f7-4c804f1fa8a4@zhaoxin.com>
Date: Mon, 26 Jun 2023 19:29:19 +0800
From: Runa Guo-oc <RunaGuo-oc@...oxin.com>
To: Damien Le Moal <dlemoal@...nel.org>,
<linux-kernel@...r.kernel.org>, <linux-ide@...r.kernel.org>
CC: <CobeChen@...oxin.com>, <TimGuo@...oxin.com>,
<TonyWWang@...oxin.com>, <LeoLiu@...oxin.com>,
<RunaGuo@...oxin.com>
Subject: Re: [PATCH] Signed-off-by: Runa Guo-oc <RunaGuo-oc@...oxin.com>
On 2023/6/16 16:34, Damien Le Moal wrote:
> On 6/16/23 16:49, Runa Guo-oc wrote:
>> [PATCH] ata:sata_zhaoxin: Add support for ZhaoXin Serial ATA
>
> Broken patch: the email subject is your SoB instead of the above line, which
> should not be part of the message (it should be the subject). Please reformat
> and resend.
>
Okay.
>>
>> Add ZhaoXin Serial ATA support for ZhaoXin CUPs.
>
> What is "ZhaoXin" ?
Zhaoxin is a Chinese company that has mastered the core technology
of independent general-purpose processors and its system platform chips,
and is committed to providing users with efficient, compatible and secure
independent general-purpose processors, chipsets and other products.
for more information, you can visit here: https://www.zhaoxin.com/.
> And what is "CUPs" ? Please spell this out.
>
Yes, this is a spelling error.
>>
>> Signed-off-by: Runa Guo-oc <RunaGuo-oc@...oxin.com>
>> ---
>> drivers/ata/Kconfig | 8 +
>> drivers/ata/Makefile | 1 +
>> drivers/ata/sata_zhaoxin.c | 384 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 393 insertions(+)
>> create mode 100644 drivers/ata/sata_zhaoxin.c
>>
>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>> index 42b51c9..ae327f3 100644
>> --- a/drivers/ata/Kconfig
>> +++ b/drivers/ata/Kconfig
>> @@ -553,6 +553,14 @@ config SATA_VITESSE
>>
>> If unsure, say N.
>>
>> +config SATA_ZHAOXIN
>> + tristate "ZhaoXin SATA support"
>> + depends on PCI
>> + help
>> + This option enables support for ZhaoXin Serial ATA.
>> +
>> + If unsure, say N.
>> +
>> comment "PATA SFF controllers with BMDMA"
>>
>> config PATA_ALI
>> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
>> index 20e6645..4b84669 100644
>> --- a/drivers/ata/Makefile
>> +++ b/drivers/ata/Makefile
>> @@ -45,6 +45,7 @@ obj-$(CONFIG_SATA_SIL) += sata_sil.o
>> obj-$(CONFIG_SATA_SIS) += sata_sis.o
>> obj-$(CONFIG_SATA_SVW) += sata_svw.o
>> obj-$(CONFIG_SATA_ULI) += sata_uli.o
>> +obj-$(CONFIG_SATA_ZHAOXIN) += sata_zhaoxin.o
>
> Please sort this alphabetically.
>
Like this?
obj-$(CONFIG_SATA_VITESSE) += sata_vsc.o
obj-$(CONFIG_SATA_ZHAOXIN) += sata_zhaoxin.o
>> obj-$(CONFIG_SATA_VIA) += sata_via.o
>> obj-$(CONFIG_SATA_VITESSE) += sata_vsc.o
>>
>> diff --git a/drivers/ata/sata_zhaoxin.c b/drivers/ata/sata_zhaoxin.c
>> new file mode 100644
>> index 0000000..ef8c73a
>> --- /dev/null
>> +++ b/drivers/ata/sata_zhaoxin.c
>> @@ -0,0 +1,384 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * sata_zhaoxin.c - ZhaoXin Serial ATA controllers
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/blkdev.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <scsi/scsi.h>
>> +#include <scsi/scsi_cmnd.h>
>> +#include <scsi/scsi_host.h>
>> +#include <linux/libata.h>
>> +
>> +#define DRV_NAME "sata_zx"
>> +#define DRV_VERSION "2.6.1"
>
> Version is not needed. The driver comes with the kernel...
>
Right, I'll remove it next time.
>> +
>> +enum board_ids_enum {
>> + zx100s,
>> +};
>> +
>> +enum {
>> + SATA_CHAN_ENAB = 0x40, /* SATA channel enable */
>> + SATA_INT_GATE = 0x41, /* SATA interrupt gating */
>> + SATA_NATIVE_MODE = 0x42, /* Native mode enable */
>> + PATA_UDMA_TIMING = 0xB3, /* PATA timing for DMA/ cable detect */
>> + PATA_PIO_TIMING = 0xAB, /* PATA timing register */
>> +
>> + PORT0 = (1 << 1),
>> + PORT1 = (1 << 0),
>> + ALL_PORTS = PORT0 | PORT1,
>> +
>> + NATIVE_MODE_ALL = (1 << 7) | (1 << 6) | (1 << 5) | (1 << 4),
>> +
>> + SATA_EXT_PHY = (1 << 6), /* 0==use PATA, 1==ext phy */
>> +};
>> +
>> +static int zx_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
>> +static int zx_scr_read(struct ata_link *link, unsigned int scr, u32 *val);
>> +static int zx_scr_write(struct ata_link *link, unsigned int scr, u32 val);
>> +static int zx_hardreset(struct ata_link *link, unsigned int *class,
>> + unsigned long deadline);
>> +
>> +static void zx_tf_load(struct ata_port *ap, const struct ata_taskfile *tf);
>> +
>> +static const struct pci_device_id zx_pci_tbl[] = {
>> + { PCI_VDEVICE(ZHAOXIN, 0x9002), zx100s },
>> + { PCI_VDEVICE(ZHAOXIN, 0x9003), zx100s },
>> +
>
> Blank line not needed.
>>> + { } /* terminate list */
>
> Comment not needed.
>
The purpose of writing like this is convenient to add new device IDs in
the future.
Considering that this is not likely, remove it also fine.
>> +};
>> +
>> +static struct pci_driver zx_pci_driver = {
>> + .name = DRV_NAME,
>> + .id_table = zx_pci_tbl,
>> + .probe = zx_init_one,
>> +#ifdef CONFIG_PM_SLEEP
>> + .suspend = ata_pci_device_suspend,
>> + .resume = ata_pci_device_resume,
>> +#endif
>> + .remove = ata_pci_remove_one,
>> +};
>> +
>> +static struct scsi_host_template zx_sht = {
>> + ATA_BMDMA_SHT(DRV_NAME),
>> +};
>> +
>> +static struct ata_port_operations zx_base_ops = {
>> + .inherits = &ata_bmdma_port_ops,
>> + .sff_tf_load = zx_tf_load,
>> +};
>> +
>> +static struct ata_port_operations zx_ops = {
>> + .inherits = &zx_base_ops,
>> + .hardreset = zx_hardreset,
>> + .scr_read = zx_scr_read,
>> + .scr_write = zx_scr_write,
>> +};
>> +
>> +static struct ata_port_info zx100s_port_info = {
>> + .flags = ATA_FLAG_SATA | ATA_FLAG_SLAVE_POSS,
>> + .pio_mask = ATA_PIO4,
>> + .mwdma_mask = ATA_MWDMA2,
>> + .udma_mask = ATA_UDMA6,
>> + .port_ops = &zx_ops,
>> +};
>> +
>> +
>
> Extra blank line not needed.
>
I see
>> +static int zx_hardreset(struct ata_link *link, unsigned int *class,
>> + unsigned long deadline)
>
> Please align the arguments together.
>
Okay.
>> +{
>> + int rc;
>> +
>> + rc = sata_std_hardreset(link, class, deadline);
>> + if (!rc || rc == -EAGAIN) {
>> + struct ata_port *ap = link->ap;
>> + int pmp = link->pmp;
>> + int tmprc;
>> +
>> + if (pmp) {
>> + ap->ops->sff_dev_select(ap, pmp);
>> + tmprc = ata_sff_wait_ready(&ap->link, deadline);
>> + } else {
>> + tmprc = ata_sff_wait_ready(link, deadline);
>> + }
>> + if (tmprc)
>> + ata_link_err(link, "COMRESET failed for wait (errno=%d)\n",
>> + rc);
>> + else
>> + ata_link_err(link, "wait for bsy success\n");
>
> Remove this. If it worked, be silent.
>
Okay.
>> +
>> + ata_link_err(link, "COMRESET success (errno=%d) ap=%d link %d\n",
>> + rc, link->ap->port_no, link->pmp);
>> + } else {
>> + ata_link_err(link, "COMRESET failed (errno=%d) ap=%d link %d\n",
>> + rc, link->ap->port_no, link->pmp);
>
> Reverse the if condition and exit early for this case. That will make the
> function code nicer. And you can drop the error message as well since
> sata_std_hardreset() prints one.
>
Yes, I agree with your. I'll adjust the above codes like these?
if(!rc || rc == -EAGAIN){
struct ata_port *ap = link->ap;
int pmp = link->pmp;
int tmprc;
if(pmp){
ap->ops->sff_dev_select(ap,pmp);
tmprc=ata_sff_wait_ready(&ap->link,deadline);
}else
tmprc=ata_sff_wait_ready(link,deadline);
if(tmprc)
ata_link_err(link,"COMRESET failed for
wait(errno=%d)\n",rc);
ata_link_err(link,"COMRESET success (errno=%d) ap=%d
link%d\n",
rc,link->ap->port_no,link->pmp);
>> + }
>> + return rc;
>> +}
>> +
>> +static int zx_scr_read(struct ata_link *link, unsigned int scr, u32 *val)
>> +{
>> + static const u8 ipm_tbl[] = { 1, 2, 6, 0 };
>> + struct pci_dev *pdev = to_pci_dev(link->ap->host->dev);
>> + int slot = 2 * link->ap->port_no + link->pmp;
>> + u32 v = 0;
>> + u8 raw;
>> +
>> + switch (scr) {
>> + case SCR_STATUS:
>> + pci_read_config_byte(pdev, 0xA0 + slot, &raw);
>> +
>> + /* read the DET field, bit0 and 1 of the config byte */
>> + v |= raw & 0x03;
>> +
>> + /* read the SPD field, bit4 of the configure byte */
>> + v |= raw & 0x30;
>> +
>> + /* read the IPM field, bit2 and 3 of the config byte */
>> + v |= ((ipm_tbl[(raw >> 2) & 0x3])<<8);
>> + break;
>> +
>> + case SCR_ERROR:
>> + /* devices other than 5287 uses 0xA8 as base */
>> + WARN_ON(pdev->device != 0x9002 && pdev->device != 0x9003);
>> + pci_write_config_byte(pdev, 0x42, slot);
>> + pci_read_config_dword(pdev, 0xA8, &v);
>> + break;
>> +
>> + case SCR_CONTROL:
>> + pci_read_config_byte(pdev, 0xA4 + slot, &raw);
>> +
>> + /* read the DET field, bit0 and bit1 */
>> + v |= ((raw & 0x02) << 1) | (raw & 0x01);
>> +
>> + /* read the IPM field, bit2 and bit3 */
>> + v |= ((raw >> 2) & 0x03) << 8;
>> +
>
> remove this blank line.
>
Okay.
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + *val = v;
>> + return 0;
>> +}
>> +
>> +static int zx_scr_write(struct ata_link *link, unsigned int scr, u32 val)
>> +{
>> + struct pci_dev *pdev = to_pci_dev(link->ap->host->dev);
>> + int slot = 2 * link->ap->port_no + link->pmp;
>> + u32 v = 0;
>> +
>> + WARN_ON(pdev == NULL);
>
> Warning about a null pointer and still dereferenceing it below is useless. The
> kernel will crash... This should not happen, right ? So remove this.
>
Okay.
>> +
>> + switch (scr) {
>> + case SCR_ERROR:
>> + /* devices 0x9002 uses 0xA8 as base */
>> + WARN_ON(pdev->device != 0x9002 && pdev->device != 0x9003);
>> + pci_write_config_byte(pdev, 0x42, slot);
>> + pci_write_config_dword(pdev, 0xA8, val);
>> + return 0;
>> +
>> + case SCR_CONTROL:
>> + /* set the DET field */
>> + v |= ((val & 0x4) >> 1) | (val & 0x1);
>> +
>> + /* set the IPM field */
>> + v |= ((val >> 8) & 0x3) << 2;
>> +
>> +
>> + pci_write_config_byte(pdev, 0xA4 + slot, v);
>> +
>> +
>
> Way too many blank lines.
>
I see
>> + return 0;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +
>> +/**
>> + * zx_tf_load - send taskfile registers to host controller
>> + * @ap: Port to which output is sent
>> + * @tf: ATA taskfile register set
>> + *
>> + * Outputs ATA taskfile to standard ATA host controller.
>> + *
>> + * This is to fix the internal bug of zx chipsets, which will
>> + * reset the device register after changing the IEN bit on ctl
>> + * register.
>> + */
>> +static void zx_tf_load(struct ata_port *ap, const struct ata_taskfile *tf)
>> +{
>> + struct ata_taskfile ttf;
>> +
>> + if (tf->ctl != ap->last_ctl) {
>> + ttf = *tf;
>> + ttf.flags |= ATA_TFLAG_DEVICE;
>> + tf = &ttf;
>
> This is very strange... Why the need for the extra local copy ? A comment would
> be nice.
>
tf, pointer to const, the content it pointed to is constant and cannot
be changed
directly.
ttf, it is a variable.
Firstly, we change its content based on *tf;
Then, make tf pointed to it;
Lastly, *tf's content will be changed undirectly.
>> + }
>> + ata_sff_tf_load(ap, tf);
>> +}
>> +
>> +static const unsigned int zx_bar_sizes[] = {
>> + 8, 4, 8, 4, 16, 256
>> +};
>> +
>> +static const unsigned int zx100s_bar_sizes0[] = {
>> + 8, 4, 8, 4, 16, 0
>> +};
>> +
>> +static const unsigned int zx100s_bar_sizes1[] = {
>> + 8, 4, 0, 0, 16, 0
>> +};
>> +
>> +static int zx_prepare_host(struct pci_dev *pdev, struct ata_host **r_host)
>> +{
>> + const struct ata_port_info *ppi0[] = {
>> + &zx100s_port_info, NULL
>> + };
>> + const struct ata_port_info *ppi1[] = {
>> + &zx100s_port_info, &ata_dummy_port_info
>> + };
>> + struct ata_host *host;
>> + int i, rc;
>> +
>> + if (pdev->device == 0x9002)
>> + rc = ata_pci_bmdma_prepare_host(pdev, ppi0, &host);
>> + else if (pdev->device == 0x9003)
>> + rc = ata_pci_bmdma_prepare_host(pdev, ppi1, &host);
>> + else
>> + rc = -EINVAL;
>> +
>
> Remove the blank line here.
>
Okay.
>> + if (rc)
>> + return rc;
>> +
>> + *r_host = host;
>> +
>> + /* 9002 hosts four sata ports as M/S of the two channels */
>> + /* 9003 hosts two sata ports as M/S of the one channel */
>
> Multi-line comment format:
>
> /*
> * ...
> * ...
> */
>
I got it.
>> + for (i = 0; i < host->n_ports; i++)
>> + ata_slave_link_init(host->ports[i]);
>> +
>> + return 0;
>> +}
>> +
>> +static void zx_configure(struct pci_dev *pdev, int board_id)
>> +{
>> + u8 tmp8;
>> +
>> + pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &tmp8);
>> + dev_info(&pdev->dev, "routed to hard irq line %d\n",
>> + (int) (tmp8 & 0xf0) == 0xf0 ? 0 : tmp8 & 0x0f);
>> +
>> + /* make sure SATA channels are enabled */
>> + pci_read_config_byte(pdev, SATA_CHAN_ENAB, &tmp8);
>> + if ((tmp8 & ALL_PORTS) != ALL_PORTS) {
>> + dev_dbg(&pdev->dev, "enabling SATA channels (0x%x)\n",
>> + (int)tmp8);
>> + tmp8 |= ALL_PORTS;
>> + pci_write_config_byte(pdev, SATA_CHAN_ENAB, tmp8);
>> + }
>> +
>> + /* make sure interrupts for each channel sent to us */
>> + pci_read_config_byte(pdev, SATA_INT_GATE, &tmp8);
>> + if ((tmp8 & ALL_PORTS) != ALL_PORTS) {
>> + dev_dbg(&pdev->dev, "enabling SATA channel interrupts (0x%x)\n",
>> + (int) tmp8);
>> + tmp8 |= ALL_PORTS;
>> + pci_write_config_byte(pdev, SATA_INT_GATE, tmp8);
>> + }
>> +
>> + /* make sure native mode is enabled */
>> + pci_read_config_byte(pdev, SATA_NATIVE_MODE, &tmp8);
>> + if ((tmp8 & NATIVE_MODE_ALL) != NATIVE_MODE_ALL) {
>> + dev_dbg(&pdev->dev,
>> + "enabling SATA channel native mode (0x%x)\n",
>> + (int) tmp8);
>> + tmp8 |= NATIVE_MODE_ALL;
>> + pci_write_config_byte(pdev, SATA_NATIVE_MODE, tmp8);
>> + }
>> +}
>> +
>> +static int zx_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>> +{
>> + unsigned int i;
>> + int rc;
>> + struct ata_host *host = NULL;
>> + int board_id = (int) ent->driver_data;
>> + const unsigned int *bar_sizes;
>> + int legacy_mode = 0;
>> +
>> + ata_print_version_once(&pdev->dev, DRV_VERSION);
>> +
>> + if (pdev->device == 0x9002 || pdev->device == 0x9003) {
>> + if ((pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
>> + u8 tmp8, mask;
>> +
>> + /* TODO: What if one channel is in native mode ... */
>
> I do not know... What about it ? If this is not expected to work/not supported,
> then return an error.
>
Yes, you're right. Zhaoxin sata controllers do not support legacy mode.
So we return an error here.
Based on the latest kernel code, this part may be adjusted like these:
u8 tmp8, mask = 0;
pci_read_config_byte(pdev, PCI_CLASS_PROG, &tmp8);
if (!ata_port_is_dummy(host->ports[0]))
mask |= (1 << 0);
if (!ata_port_is_dummy(host->ports[1]))
mask |= (1 << 2);
if ((tmp8 & mask) != mask)
legacy_mode = 1;
>> + pci_read_config_byte(pdev, PCI_CLASS_PROG, &tmp8);
>> + mask = (1 << 2) | (1 << 0);
>> + if ((tmp8 & mask) != mask)
>> + legacy_mode = 1;
>> + }
>> + if (legacy_mode)
>> + return -EINVAL;
>> + }
>> +
>> + rc = pcim_enable_device(pdev);
>> + if (rc)
>> + return rc;
>> +
>> + if (board_id == zx100s && pdev->device == 0x9002)
>> + bar_sizes = &zx100s_bar_sizes0[0];
>> + else if (board_id == zx100s && pdev->device == 0x9003)
>> + bar_sizes = &zx100s_bar_sizes1[0];
>> + else
>> + bar_sizes = &zx_bar_sizes[0];
>> +
>> + for (i = 0; i < ARRAY_SIZE(zx_bar_sizes); i++) {
>> + if ((pci_resource_start(pdev, i) == 0) ||
>> + (pci_resource_len(pdev, i) < bar_sizes[i])) {
>> + if (bar_sizes[i] == 0)
>> + continue;
>> +
>> + dev_err(&pdev->dev,
>> + "invalid PCI BAR %u (sz 0x%llx, val 0x%llx)\n",
>> + i,
>> + (unsigned long long)pci_resource_start(pdev, i),
>> + (unsigned long long)pci_resource_len(pdev, i));
>> +
>> + return -ENODEV;
>> + }
>> + }
>> +
>> + switch (board_id) {
>> + case zx100s:
>> + rc = zx_prepare_host(pdev, &host);
>> + break;
>> + default:
>> + rc = -EINVAL;
>> + }
>> + if (rc)
>> + return rc;
>> +
>> + zx_configure(pdev, board_id);
>> +
>> + pci_set_master(pdev);
>> + return ata_host_activate(host, pdev->irq, ata_bmdma_interrupt,
>> + IRQF_SHARED, &zx_sht);
>> +}
>> +
>> +module_pci_driver(zx_pci_driver);
>> +
>> +MODULE_AUTHOR("Yanchen:YanchenSun@...oxin.com");
>> +MODULE_DESCRIPTION("SCSI low-level driver for ZX SATA controllers");
>
> This is not a scsi driver...
>
I treat it as a scsi driver for the following reasons, which may be not
accurate.
1, IO path: vfs->fs->block layer->scsi layer->this driver;
2, Extracted from the following link:
"SCSI Lower level drivers (LLDs) are variously called host bus adapter
(HBA) drivers and host drivers (HD)."
https://www.kernel.org/doc/html/latest/scsi/scsi_mid_low_api.html
Maybe I shall delete it next time.
>> +MODULE_LICENSE("GPL");
>> +MODULE_DEVICE_TABLE(pci, zx_pci_tbl);
>> +MODULE_VERSION(DRV_VERSION);
>
Powered by blists - more mailing lists