linux-kernel - Re: [PATCH v4 5/5] scsi: ufs: ufs-mediatek: Add MCQ support for MTK platform

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Mar 2023 03:29:04 +0000
From:   Powen Kao (高伯文) <Powen.Kao@...iatek.com>
To:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "bvanassche@....org" <bvanassche@....org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Stanley Chu (朱原陞) 
        <stanley.chu@...iatek.com>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>
CC:     Peter Wang (王信友) 
        <peter.wang@...iatek.com>,
        Eddie Huang (黃智傑) 
        <eddie.huang@...iatek.com>,
        Jiajie Hao (郝加节) 
        <jiajie.hao@...iatek.com>,
        CC Chou (周志杰) <cc.chou@...iatek.com>,
        Alice Chao (趙珮均) 
        <Alice.Chao@...iatek.com>,
        wsd_upstream <wsd_upstream@...iatek.com>,
        Chun-Hung Wu (巫駿宏) 
        <Chun-hung.Wu@...iatek.com>,
        Chaotian Jing (井朝天) 
        <Chaotian.Jing@...iatek.com>,
        Naomi Chu (朱詠田) <Naomi.Chu@...iatek.com>,
        Mason Zhang (章辉) <Mason.Zhang@...iatek.com>
Subject: Re: [PATCH v4 5/5] scsi: ufs: ufs-mediatek: Add MCQ support for MTK
 platform

On Mon, 2023-03-13 at 14:36 -0700, Bart Van Assche wrote:
> On 3/6/23 22:54, Po-Wen Kao wrote:
> > +static unsigned int mtk_mcq_irq[UFSHCD_MAX_Q_NR];
> 
> Shouldn't there be one instance of this array per controller such
> that 
> this driver can support multiple host controllers instead of only
> one?
> 

True, I will fix the flow to get irq after `struct ufs_hba` is
allocated so that these infomation can be stored as per host instance. 

> > -	err = ufshcd_make_hba_operational(hba);
> > +	if (!hba->mcq_enabled) {
> > +		err = ufshcd_make_hba_operational(hba);
> > +	} else {
> > +		ufs_mtk_config_mcq(hba, false);
> > +		ufshcd_mcq_make_queues_operational(hba);
> > +		ufshcd_mcq_config_mac(hba, hba->nutrs);
> > +		ufshcd_writel(hba, ufshcd_readl(hba, REG_UFS_MEM_CFG) |
> > 0x1,
> > +			      REG_UFS_MEM_CFG);
> > +	}
> 
> ufshcd_config_mcq() in the UFSHCD core already calls 
> ufshcd_mcq_config_mac(). Why is there another call to 
> ufshcd_mcq_config_mac() in the MediaTek driver?

MCQ configuration will be reset through HCE cycle on our host
controller, hence we need to reconfigure those registers. 

> 
> > +	/*
> > +	 * Disable MCQ_CQ_EVENT interrupt.
> > +	 * Use CQ Tail Entry Push Status instead.
> > +	 */
> > +	ufshcd_disable_intr(hba, MCQ_CQ_EVENT_STATUS);
> 
> UFS host controller drivers should not call ufshcd_disable_intr().
> 
>  From the UFSHCI 4.0 specification: "MCQ CQ Event Status (CQES):
> This 
> bit is transparent and becomes ‘1’ when all of the following
> conditions 
> are met:
> • Controller is operating in MCQ mode (Config.QT=1)
> • ESI is not enabled (Config.ESIE=0)
> • CQES set only for Events in Queues that do not have interrupt 
> aggregation enabled or the Events that do not belong to
> MCQIACRy.IACTH 
> counter operation criteria.
> • At least one bit in CQISy is set and associated bit in CQIEy is
> set. 
> y=0..31"
> 
> Is there perhaps a bug in the MediaTek controller that causes the MCQ
> CQ 
> Event Status to be set in ESI mode? If not, can the above 
> ufshcd_disable_intr() call be left out?

We did not implement ESI at hardware level but per queue hw interrupt.
Without disabling MCQ_CQ_EVENT_STATUS, there will be two interrupts,
CQES (traditional interrupt) and CQ Tail Entry Push Interrupt (per
queue hw interrupt), raised on a signle command arrival.

I wouldn't consider it as a bug, but different interrupt design.
> 
> Thanks,
> 
> Bart.
> 

Thanks for your review

Powen