> -----Original Message-----
> From: linux-scsi-owner@xxxxxxxxxxxxxxx <linux-scsi-owner@xxxxxxxxxxxxxxx>
> On Behalf Of Christoph Hellwig
> Sent: Tuesday, September 04, 2018 10:12 PM
> To: Avri Altman <Avri.Altman@xxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>; Johannes Thumshirn
> <jthumshirn@xxxxxxx>; Hannes Reinecke <hare@xxxxxxxx>; Bart Van Assche
> <Bart.VanAssche@xxxxxxx>; James E.J. Bottomley
> <jejb@xxxxxxxxxxxxxxxxxx>; Martin K. Petersen
> <martin.petersen@xxxxxxxxxx>; linux-scsi@xxxxxxxxxxxxxxx; Stanislav Nijnikov
> <Stanislav.Nijnikov@xxxxxxx>; Avi Shchislowski
> <Avi.Shchislowski@xxxxxxx>; Alex Lemberg <Alex.Lemberg@xxxxxxx>;
> Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>; Vinayak Holikatti
> <Vinayak.Holikatti@xxxxxxx>
> Subject: Re: [PATCH v3 3/7] scsi: ufs: Add fill task management request
>
> In general this looks good, but a question below:
>
> > index ed37914..d18832a 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -5598,6 +5598,32 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba
> *hba, int tag)
> > return err;
> > }
> >
> > +static void ufshcd_fill_tm_req(struct utp_task_req_desc *task_req_descp,
> > + int lun_id, int task_id, u8 tm_function,
> > + int task_tag)
> > +{
> > + struct utp_upiu_task_req *task_req_upiup;
> > +
> > + /* Configure task request descriptor */
> > + task_req_descp->header.dword_0 =
> cpu_to_le32(UTP_REQ_DESC_INT_CMD);
> > + task_req_descp->header.dword_2 =
> > + cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
> > +
> > + task_req_upiup =
> > + (struct utp_upiu_task_req *)task_req_descp->task_req_upiu;
>
>
> Why is task_req_upiu a __le32 array instead of using the proper
> structure?
Looking into the UFSHCI spec (JESD223C March 2016) paragraph 6.2,
It doesn't specify any inner structure of the task management
request or response, just a bunch of 8 DW each.
I guess this is why it is defined as a __le32 array.
So the host controller is not aware of the inner structure of this
Sequence, and probably shouldn't. Making it aware of that,
e.g. by moving utp_upiu_task_req and utp_upiu_task_rsp
from ufs.h to ufshci.h will break this abstraction.
>
> Doing that would nicely clean up the code:
>
> static void ufshcd_fill_tm_req(struct utp_task_req_desc *treq,
> int lun_id, int task_id, u8 tm_function,
> int task_tag)
> {
> /* Configure task request descriptor */
> treq->header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
> treq->header.dword_2 =
> cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
>
> treq->task_req_upiup->header.dword_0 =
> UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0,
> lun_id,
> task_tag);
> treq->task_req_upiup->header.dword_1 =
> UPIU_HEADER_DWORD(0, tm_function, 0, 0);
> treq->task_req_upiup->input_param1 = cpu_to_be32(lun_id);
> treq->task_req_upiup->input_param2 = cpu_to_be32(task_id);
> }
