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

[Runa Guo-oc]

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@xxxxxxxxxxx>
>>>> ---
>>>>  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@xxxxxxxxxxx");
>>>> +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.