On 2/11/22 21:25, John Garry wrote:
> On 11/02/2022 07:37, Damien Le Moal wrote:
>> Introduce the pm8001_ccb_alloc() and pm8001_ccb_free() helpers to
>> replace the typical code pattern:
>>
>> res = pm8001_tag_alloc(pm8001_ha, &ccb_tag);
>> ...
>> ccb = &pm8001_ha->ccb_info[ccb_tag];
>> ccb->device = pm8001_ha_dev;
>> ccb->ccb_tag = ccb_tag;
>> ccb->task = task;
>> ccb->n_elem = 0;
>>
>> With a simpler single function call:
>>
>> ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_ha_dev, task);
>>
>> The pm8001_ccb_alloc() helper ensures that all fields of the ccb info
>> structure for the newly allocated tag are all initialized, except the
>> buf_prd field. All call site of the pm8001_tag_alloc() function that
>> use the ccb info associated with the allocated tag are converted to use
>> the new helpers.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/scsi/pm8001/pm8001_hwi.c | 153 ++++++++++++++-----------------
>> drivers/scsi/pm8001/pm8001_sas.c | 37 +++-----
>> drivers/scsi/pm8001/pm8001_sas.h | 33 +++++++
>> drivers/scsi/pm8001/pm80xx_hwi.c | 66 ++++++-------
>> 4 files changed, 141 insertions(+), 148 deletions(-)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
>> index d853e8d0195a..8c4cf4e254ba 100644
>> --- a/drivers/scsi/pm8001/pm8001_hwi.c
>> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
>> @@ -1757,8 +1757,6 @@ int pm8001_handle_event(struct pm8001_hba_info *pm8001_ha, void *data,
>> static void pm8001_send_abort_all(struct pm8001_hba_info *pm8001_ha,
>> struct pm8001_device *pm8001_ha_dev)
>> {
>> - int res;
>> - u32 ccb_tag;
>> struct pm8001_ccb_info *ccb;
>> struct sas_task *task = NULL;
>> struct task_abort_req task_abort;
>> @@ -1780,28 +1778,26 @@ static void pm8001_send_abort_all(struct pm8001_hba_info *pm8001_ha,
>>
>> task->task_done = pm8001_task_done;
>>
>> - res = pm8001_tag_alloc(pm8001_ha, &ccb_tag);
>> - if (res)
>> + ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_ha_dev, task);
>> + if (!ccb) {
>> + pm8001_dbg(pm8001_ha, FAIL, "cannot allocate tag !!!\n");
>
> Should we print this always and move it into pm8001_ccb_alloc()?
Good idea. Will do.
[...]
>> @@ -4433,7 +4424,7 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>> u32 stp_sspsmp_sata = 0x4;
>> struct inbound_queue_table *circularQ;
>> u32 linkrate, phy_id;
>> - int rc, tag = 0xdeadbeef;
>> + int rc;
>> struct pm8001_ccb_info *ccb;
>> u8 retryFlag = 0x1;
>> u16 firstBurstSize = 0;
>> @@ -4444,13 +4435,11 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>
> I think that this needs to be fixed to release the ccb when
> pm8001_mip_build_cmd() fails (not shown).
OK. Will do.
[...]
>> +/*
>> + * Allocate a new tag and return the corresponding ccb after initializing it.
>> + */
>> +static inline struct pm8001_ccb_info *
>> +pm8001_ccb_alloc(struct pm8001_hba_info *pm8001_ha,
>> + struct pm8001_device *dev, struct sas_task *task)
>> +{
>> + struct pm8001_ccb_info *ccb;
>> + u32 tag;
>> +
>> + if (pm8001_tag_alloc(pm8001_ha, &tag))
>> + return NULL;
>> +
>> + ccb = &pm8001_ha->ccb_info[tag];
>> + ccb->task = task;
>> + ccb->n_elem = 0;
>> + ccb->ccb_tag = tag;
>> + ccb->device = dev;
>> + ccb->fw_control_context = NULL;
>> + ccb->open_retry = 0;
>
> I'd just memset the whole thing to be sure (paranoid). And I think that
> we are also missing clearing the ccb_dma_handle.
Can't do that. The ccb buf_prd is allocated on controller init and stays
until teardown. I know, i did that first and got a crash :)
>
>> +
>> + return ccb;
>> +}
>> +
>> +/*
>
> Thanks,
> John
--
Damien Le Moal
Western Digital Research
