Re: [PATCH] Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com>

[Damien Le Moal <dlemoal@kernel.org>]

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.

> 
>> And what is "CUPs" ? Please spell this out.
>>
> 
> Yes, this is  a spelling error.
> 
>>>
>>> Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.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.

>>> +	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.


[...]

>>> +MODULE_AUTHOR("Yanchen:YanchenSun@zhaoxin.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.

-- 
Damien Le Moal
Western Digital Research