Bart, thanks. > -----Original Message----- > From: Bart Van Assche > Sent: Wednesday, August 01, 2018 6:28 PM > To: [email protected]; Avri Altman ; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected] > Cc: Vinayak Holikatti ; Avi Shchislowski ; Alex Lemberg ; Stanislav Nijnikov ; > [email protected] > Subject: Re: [PATCH 4/6] scsi: ufs: Add API to execute raw upiu commands > > On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote: > > + wait_event(hba->tm_tag_wq, ufshcd_get_tm_free_slot(hba, > &free_slot)); > > The above is the weirdest API I have seen so far for tag allocation. > Why does ufshcd_get_tm_free_slot() does a linear search through a bitmap > instead of using the sbitmap data structure? See my answer below.
> > > + ufshcd_writel(hba, 1 << free_slot, > REG_UTP_TASK_REQ_DOOR_BELL); > > + /* Make sure that doorbell is committed immediately */ > > + wmb(); > > This is the first time that I see someone claiming that issuing a write memory > barrier causes the write to be executed faster than without memory barrier. > Are you sure that comment is correct and that that wmb() is necessary? See my answer below > > > + spin_unlock_irqrestore(host->host_lock, flags); > > + > > + /* wait until the task management command is completed */ > > + err = wait_event_timeout(hba->tm_wq, > > + test_bit(free_slot, &hba->tm_condition), > > + msecs_to_jiffies(TM_CMD_TIMEOUT)); > > Did you perhaps start implementing the ufshcd_issue_tm_upiu_cmd() > function by > copy/pasting ufshcd_issue_tm_cmd()? Please don't do that and instead avoid > code > duplication by moving shared code in a new function. Yes I did. I wanted to avoid changing any of the driver's core functionality, just adding the new one. And yes, this weird tagging mechanism and strange comments are just part of ufshcd_issue_tm_cmd(). I will try to do what you said. Thanks. > > > + /* ignore the returning value here - ufshcd_check_query_response is > > + * bound to fail since dev_cmd.query and dev_cmd.type were left > empty. > > + * read the response directly ignoring all errors. > > + */ > > Have you verified this patch with checkpatch? This is not the proper kernel > comment style. Yes I did. Anyway, as this patch is changing significantly, will run it again. > > Thanks, > > Bart.
