| 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: <966a1048-cd14-d796-8b9d-734605796652@huawei.com>
Date: Fri, 4 Mar 2022 09:41:22 +0000
From: John Garry <john.garry@...wei.com>
To: Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
<jejb@...ux.ibm.com>, <martin.petersen@...cle.com>,
<jinpu.wang@...ud.ionos.com>
CC: <linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<Ajish.Koshy@...rochip.com>, <linuxarm@...wei.com>,
<Viswas.G@...rochip.com>, <hch@....de>, <liuqi115@...wei.com>,
<chenxiang66@...ilicon.com>
Subject: Re: [PATCH 3/4] scsi: pm8001: Use libsas internal abort support
On 03/03/2022 16:36, Damien Le Moal wrote:
>> - rc = send_task_abort(pm8001_ha, opc, device_id, flag,
>> - task_tag, cmd_tag);
>> + rc = send_task_abort(pm8001_ha, opc, device_id, abort->type,
>> + abort->tag, ccb->ccb_tag);
> Nit: Can you indent this together with "(" ? I find it easier to read:)
ok, I can align it.
>
>> if (rc != TMF_RESP_FUNC_COMPLETE)
>> pm8001_dbg(pm8001_ha, EH, "rc= %d\n", rc);
>> return rc;
>> diff --git a/drivers/scsi/pm8001/pm8001_hwi.h b/drivers/scsi/pm8001/pm8001_hwi.h
>> index d1f3aa93325b..961d0465b923 100644
>> --- a/drivers/scsi/pm8001/pm8001_hwi.h
>> +++ b/drivers/scsi/pm8001/pm8001_hwi.h
>> @@ -434,11 +434,6 @@ struct task_abort_req {
>> u32 reserved[11];
>> } __attribute__((packed, aligned(4)));
>>
>> -/* These flags used for SSP SMP & SATA Abort */
>> -#define ABORT_MASK 0x3
>> -#define ABORT_SINGLE 0x0
>> -#define ABORT_ALL 0x1
>> -
>> /**
>> * brief the data structure of SSP SATA SMP Abort Response
>> * use to describe SSP SMP & SATA Abort Response ( 64 bytes)
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
>> index ac9c40c95070..d1224674173e 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.c
>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>> @@ -324,6 +324,18 @@ static int pm8001_task_prep_ata(struct pm8001_hba_info *pm8001_ha,
>> return PM8001_CHIP_DISP->sata_req(pm8001_ha, ccb);
>> }
>>
>> +/**
>> + * pm8001_task_prep_internal_abort - the dispatcher function, prepare data
>> + * for internal abort task
>> + * @pm8001_ha: our hba card information
>> + * @ccb: the ccb which attached to sata task
>> + */
>> +static int pm8001_task_prep_internal_abort(struct pm8001_hba_info *pm8001_ha,
>> + struct pm8001_ccb_info *ccb)
>> +{
>> + return PM8001_CHIP_DISP->task_abort(pm8001_ha, ccb);
>> +}
>> +
>> /**
>> * pm8001_task_prep_ssp_tm - the dispatcher function, prepare task management data
>> * @pm8001_ha: our hba card information
>> @@ -367,6 +379,43 @@ static int sas_find_local_port_id(struct domain_device *dev)
>> #define DEV_IS_GONE(pm8001_dev) \
>> ((!pm8001_dev || (pm8001_dev->dev_type == SAS_PHY_UNUSED)))
>>
>> +
>> +static int pm8001_deliver_command(struct pm8001_hba_info *pm8001_ha,
>> + struct pm8001_ccb_info *ccb)
>> +{
>> + struct sas_task *task = ccb->task;
>> + enum sas_protocol task_proto = task->task_proto;
>> + struct sas_tmf_task *tmf = task->tmf;
>> + int is_tmf = !!tmf;
>> + int rc;
>> +
>> + switch (task_proto) {
>> + case SAS_PROTOCOL_SMP:
>> + rc = pm8001_task_prep_smp(pm8001_ha, ccb);
>> + break;
>> + case SAS_PROTOCOL_SSP:
>> + if (is_tmf)
>> + rc = pm8001_task_prep_ssp_tm(pm8001_ha, ccb, tmf);
>> + else
>> + rc = pm8001_task_prep_ssp(pm8001_ha, ccb);
>> + break;
>> + case SAS_PROTOCOL_SATA:
>> + case SAS_PROTOCOL_STP:
>> + rc = pm8001_task_prep_ata(pm8001_ha, ccb);
>> + break;
>> + case SAS_PROTOCOL_INTERNAL_ABORT:
>> + rc = pm8001_task_prep_internal_abort(pm8001_ha, ccb);
>> + break;
>> + default:
>> + dev_err(pm8001_ha->dev, "unknown sas_task proto: 0x%x\n",
>> + task_proto);
>> + rc = -EINVAL;
>> + break;
>> + }
>> +
>> + return rc;
> rc variable is not very useful here. Why not use return directly for each case ?
ok, I can make that change.
>
>> +}
>> +
>> /**
>> * pm8001_queue_command - register for upper layer used, all IO commands sent
>> * to HBA are from this interface.
>> @@ -379,16 +428,15 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
>> enum sas_protocol task_proto = task->task_proto;
>> struct domain_device *dev = task->dev;
>> struct pm8001_device *pm8001_dev = dev->lldd_dev;
>> + bool internal_abort = sas_is_internal_abort(task);
>> struct pm8001_hba_info *pm8001_ha;
>> struct pm8001_port *port = NULL;
>> struct pm8001_ccb_info *ccb;
>> - struct sas_tmf_task *tmf = task->tmf;
>> - int is_tmf = !!task->tmf;
>> unsigned long flags;
>> u32 n_elem = 0;
>> int rc = 0;
>>
>> - if (!dev->port) {
>> + if (!internal_abort && !dev->port) {
>> ts->resp = SAS_TASK_UNDELIVERED;
>> ts->stat = SAS_PHY_DOWN;
>> if (dev->dev_type != SAS_SATA_DEV)
>> @@ -410,7 +458,8 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
>> pm8001_dev = dev->lldd_dev;
>> port = &pm8001_ha->port[sas_find_local_port_id(dev)];
>>
>> - if (DEV_IS_GONE(pm8001_dev) || !port->port_attached) {
>> + if (!internal_abort && (DEV_IS_GONE(pm8001_dev) ||
>> + !port->port_attached)) {
> Wrapping the line after "&&" would make this a lot cleaner and easier to read.
Agreed, I can do it.
But if you can see any neater ways to skip these checks for internal
abort in the common queue command path then I would be glad to hear it.
>
>> ts->resp = SAS_TASK_UNDELIVERED;
>> ts->stat = SAS_PHY_DOWN;
>> if (sas_protocol_ata(task_proto)) {
>> @@ -448,27 +497,7 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
>>
Thanks,
John
Powered by blists - more mailing lists