linux-kernel - Re: [PATCH v7 3/7] i2c: mediatek: Dump i2c/dma register when a timeout occurs

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 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:   Fri, 8 Oct 2021 15:13:21 +0800
From:   Kewei Xu <kewei.xu@...iatek.com>
To:     Wolfram Sang <wsa@...-dreams.de>
CC:     <matthias.bgg@...il.com>, <robh+dt@...nel.org>,
        <linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>,
        <srv_heupstream@...iatek.com>, <leilk.liu@...iatek.com>,
        <qii.wang@...iatek.com>, <liguo.zhang@...iatek.com>,
        <caiyu.chen@...iatek.com>, <ot_daolong.zhu@...iatek.com>,
        <yuhan.wei@...iatek.com>
Subject: Re: [PATCH v7 3/7] i2c: mediatek: Dump i2c/dma register when a
 timeout occurs

On Sat, 2021-10-02 at 08:37 +0200, Wolfram Sang wrote:
> > @@ -837,6 +839,57 @@ static int mtk_i2c_set_speed(struct mtk_i2c
> > *i2c, unsigned int parent_clk)
> >  	return 0;
> >  }
> > +static void i2c_dump_register(struct mtk_i2c *i2c)
> > +{
> > +	dev_err(i2c->dev, "SLAVE_ADDR: 0x%x, INTR_MASK: 0x%x\n",
> > +		mtk_i2c_readw(i2c, OFFSET_SLAVE_ADDR),
> > +		mtk_i2c_readw(i2c, OFFSET_INTR_MASK));
> 
> I think this is too verbose and should be a debugging only patch not
> really suited for upstream. But if you like it this way, then keep
> the verbosity. However, dev_err is too strong, this really needs to
> be
> dev_dbg. Timeouts can happen on an I2C bus, think about an EEPROM in
> a
> long erase cycle while you want to read it. Perfectly normal.
> 
> 
> >  	if (ret == 0) {
> > -		dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", msgs-
> > >addr);
> > +		dev_err(i2c->dev, "addr: %x, transfer timeout\n", msgs-
> > >addr);
> > +		i2c_dump_register(i2c);
> 
> Needs to stay dev_dbg as well.
> 
> Yes, It is used for debugging,but dump the value of value of the
> register is very important for debugging,so we think it is
> necessary. We will use dev_dbg to replace dev_err in V8. Thanks~