| 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
| ||
|
Date: Tue, 27 Jun 2023 15:17:03 +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/26 20:40, Damien Le Moal wrote:
> On 6/26/23 20:29, Runa Guo-oc wrote:
>> 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/.
>
> A company marketing message is not very informative, technically speaking. What
> is this chipset and on what board/machine can it be found ? That is the more
> relevant information we need in this commit message.
>
The reason why it is called Zhaoxin SATA is actually to express that it
applies
to all Zhaoxin support of its separate chipset/SOC, for example,
ZX-100S/ZX-200
chipsets.
>>
>>> 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"
>
> Same here. If ZhaoXin is only a company name, we need also a chipset model to be
> informative regarding which HW this driver serves.
>
As mentioned before, the chipset models this driver serves are ZX-100S
and ZX-200
currently.
>>>> + 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
>
> Yes.
>
>>>> +
>>>> + 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);
>>
>
> You did not understand my point. Doing:
>
> rc = sata_std_hardreset(link, class, deadline);
> if (rc && rc != -EAGAIN) {
> ata_link_err(link, "COMRESET failed (errno=%d) ap=%d link %d\n",
> rc, link->ap->port_no, link->pmp);
> return rc;
> }
>
> /* rc == 0 || rc == -EAGAIN case */
> ...
>
> Makes the code much nicer.
>
>
> [...]
>
Okay, I'll adjust it like this?
...
struct ata_port *ap = link->ap;
int pmp = link->pmp;
int tmprc;
rc = sata_std_hardreset(link, class, deadline);
if (rc && rc != -EAGAIN) {
ata_link_err(link, "COMRESET failed (errno=%d) ap=%d link %d\n",
rc, link->ap->port_no, link->pmp);
return rc;
}
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);
return rc;
>>>> +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)."
>
> Again, this is not a scsi driver. Even if in Linux all ATA drives seat under the
> scsi layer, this is an ATA subsytem driver for an ATA drive. Not SCSI.
>
Okay, I'll remove this inaccurate description next time.
Thank you.
Powered by blists - more mailing lists