Bart, thanks. +AD4- -----Original Message----- +AD4- From: Bart Van Assche +AD4- Sent: Wednesday, August 01, 2018 6:28 PM +AD4- To: hch+AEA-lst.de+ADs- Avri Altman +ADs- linux-scsi+AEA-vger.kernel.org+ADs- +AD4- jthumshirn+AEA-suse.de+ADs- hare+AEA-suse.com+ADs- martin.petersen+AEA-oracle.com+ADs- +AD4- jejb+AEA-linux.vnet.ibm.com +AD4- Cc: Vinayak Holikatti +ADs- Avi Shchislowski +ADs- Alex Lemberg +ADs- Stanislav Nijnikov +ADs- +AD4- subhashj+AEA-codeaurora.org +AD4- Subject: Re: +AFs-PATCH 4/6+AF0- scsi: ufs: Add API to execute raw upiu commands +AD4- +AD4- On Wed, 2018-08-01 at 11:04 +-0300, Avri Altman wrote: +AD4- +AD4- +- wait+AF8-event(hba-+AD4-tm+AF8-tag+AF8-wq, ufshcd+AF8-get+AF8-tm+AF8-free+AF8-slot(hba, +AD4- +ACY-free+AF8-slot))+ADs- +AD4- +AD4- The above is the weirdest API I have seen so far for tag allocation. +AD4- Why does ufshcd+AF8-get+AF8-tm+AF8-free+AF8-slot() does a linear search through a bitmap +AD4- instead of using the sbitmap data structure? See my answer below. +AD4- +AD4- +AD4- +- ufshcd+AF8-writel(hba, 1 +ADwAPA- free+AF8-slot, +AD4- REG+AF8-UTP+AF8-TASK+AF8-REQ+AF8-DOOR+AF8-BELL)+ADs- +AD4- +AD4- +- /+ACo- Make sure that doorbell is committed immediately +ACo-/ +AD4- +AD4- +- wmb()+ADs- +AD4- +AD4- This is the first time that I see someone claiming that issuing a write memory +AD4- barrier causes the write to be executed faster than without memory barrier. +AD4- Are you sure that comment is correct and that that wmb() is necessary? See my answer below +AD4- +AD4- +AD4- +- spin+AF8-unlock+AF8-irqrestore(host-+AD4-host+AF8-lock, flags)+ADs- +AD4- +AD4- +- +AD4- +AD4- +- /+ACo- wait until the task management command is completed +ACo-/ +AD4- +AD4- +- err +AD0- wait+AF8-event+AF8-timeout(hba-+AD4-tm+AF8-wq, +AD4- +AD4- +- test+AF8-bit(free+AF8-slot, +ACY-hba-+AD4-tm+AF8-condition), +AD4- +AD4- +- msecs+AF8-to+AF8-jiffies(TM+AF8-CMD+AF8-TIMEOUT))+ADs- +AD4- +AD4- Did you perhaps start implementing the ufshcd+AF8-issue+AF8-tm+AF8-upiu+AF8-cmd() +AD4- function by +AD4- copy/pasting ufshcd+AF8-issue+AF8-tm+AF8-cmd()? Please don't do that and instead avoid +AD4- code +AD4- 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+AF8-issue+AF8-tm+AF8-cmd(). I will try to do what you said. Thanks. +AD4- +AD4- +AD4- +- /+ACo- ignore the returning value here - ufshcd+AF8-check+AF8-query+AF8-response is +AD4- +AD4- +- +ACo- bound to fail since dev+AF8-cmd.query and dev+AF8-cmd.type were left +AD4- empty. +AD4- +AD4- +- +ACo- read the response directly ignoring all errors. +AD4- +AD4- +- +ACo-/ +AD4- +AD4- Have you verified this patch with checkpatch? This is not the proper kernel +AD4- comment style. Yes I did. Anyway, as this patch is changing significantly, will run it again. +AD4- +AD4- Thanks, +AD4- +AD4- Bart.
