linux-kernel - Re: [PATCH] i2c: davinci: Add block read functionality for IPMI

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]

Message-ID: <20140522090041.GA2573@katana>
Date:	Thu, 22 May 2014 11:00:41 +0200
From:	Wolfram Sang <wsa@...-dreams.de>
To:	"Karicheri, Muralidharan" <m-karicheri2@...com>
Cc:	"davinci-linux-open-source@...ux.davincidsp.com" 
	<davinci-linux-open-source@...ux.davincidsp.com>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Ding, Garrett" <g-ding@...com>, "Nori, Sekhar" <nsekhar@...com>,
	Kevin Hilman <khilman@...prootsystems.com>,
	"Shilimkar, Santosh" <santosh.shilimkar@...com>
Subject: Re: [PATCH] i2c: davinci: Add block read functionality for IPMI

Hi,

thanks for the patch.

> >+/* capabilities */
> >+#define I2C_CAPABILITIES	(I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | \
> >+				 I2C_FUNC_SMBUS_READ_BLOCK_DATA)

I don't see the need for a seperate define.

> >+
> > struct davinci_i2c_dev {
> > 	struct device           *dev;
> > 	void __iomem		*base;
> >@@ -318,7 +322,13 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg
> >*msg, int stop)
> > 	davinci_i2c_write_reg(dev, DAVINCI_I2C_SAR_REG, msg->addr);
> >
> > 	dev->buf = msg->buf;
> >-	dev->buf_len = msg->len;
> >+
> >+	 /* if first received byte is length, set buf_len = 0xffff as flag */
> >+	if (msg->flags & I2C_M_RECV_LEN)
> >+		dev->buf_len = 0xffff;

a) this magic value should be a define instead of a comment
b) i2c messages easily have a 16 bit range, so 0xffff is a troublesome
choice.

> >+	else
> >+		dev->buf_len = msg->len;
> >+
> > 	dev->stop = stop;
> >
> > 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CNT_REG, dev->buf_len); @@ -456,7
> >+466,7 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >
> > static u32 i2c_davinci_func(struct i2c_adapter *adap)  {
> >-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> >+	return I2C_CAPABILITIES;
> > }
> >
> > static void terminate_read(struct davinci_i2c_dev *dev) @@ -528,10 +538,32 @@ static
> >irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
> >
> > 		case DAVINCI_I2C_IVR_RDR:
> > 			if (dev->buf_len) {
> >-				*dev->buf++ =
> >-				    davinci_i2c_read_reg(dev,
> >-							 DAVINCI_I2C_DRR_REG);
> >+				*dev->buf++ = davinci_i2c_read_reg(dev,
> >+							DAVINCI_I2C_DRR_REG);
> >+				/*
> >+				 * check if the first received byte is message
> >+				 * length, i.e, I2C_M_RECV_LEN
> >+				 */
> >+				if (dev->buf_len == 0xffff)
> >+					dev->buf_len = *(dev->buf - 1) + 1;

Please rework the code to get rid of the '- 1' and '+ 1'. They look
hackish and make the code less readable.

> >+
> > 				dev->buf_len--;
> >+				/*
> >+				 * send NACK/STOP bits BEFORE last byte is
> >+				 * received
> >+				 */
> >+				if (dev->buf_len == 1) {
> >+					w = davinci_i2c_read_reg(dev,
> >+							DAVINCI_I2C_MDR_REG);
> >+					w |= DAVINCI_I2C_MDR_NACK;
> >+					davinci_i2c_write_reg(dev,
> >+							DAVINCI_I2C_MDR_REG, w);
> >+
> >+					w |= DAVINCI_I2C_MDR_STP;
> >+					davinci_i2c_write_reg(dev,
> >+							DAVINCI_I2C_MDR_REG, w);
> >+				}
> >+

Looks like an unreleated change to me? Why is this I2C_M_RECV_LEN
specific?

Kind regards,

   Wolfram

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)