linux-kernel - RE: [PATCH V2 2/4] i2c: tegra: Update I2C transfer using buffer

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:   Fri, 25 Jan 2019 23:26:57 +0000
From:   Sowjanya Komatineni <skomatineni@...dia.com>
To:     Dmitry Osipenko <digetx@...il.com>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Mantravadi Karthik <mkarthik@...dia.com>,
        Shardar Mohammed <smohammed@...dia.com>,
        Timo Alho <talho@...dia.com>
CC:     "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: RE: [PATCH V2 2/4] i2c: tegra: Update I2C transfer using buffer


> >>>> +	buffer = kmalloc(ALIGN(msg->len, BYTES_PER_FIFO_WORD) +
> >>>> +					I2C_PACKET_HEADER_SIZE, GFP_KERNEL);
> >>>> +	if (!buffer)
> >>>> +		return -ENOMEM;
> >>>
> >>> Isn't it possible to avoid "buffer" allocation / copying overhead and keep code as-is for the PIO mode?
> >>>
> >>
> >> Keeping PIO mode code as is and adding DMA mode will have redundant code for header generation and msg complete timeout sections.
> >> Also in existing PIO mode implementation, TX FIFO is filled as soon as the word is ready but for DMA mode need to put all msg bytes along with header together to send thru DMA.
> >> So created common buffer to perform PIO/DMA to reduce redundancy.
> >>
> > 
> > Okay, what about something like in this sketch:
> > 
> > ...
> > 	if (msg->flags & I2C_M_RD)
> > 		xfer_size = msg->len;
> > 	else
> > 		xfer_size = ALIGN(msg->len, BYTES_PER_FIFO_WORD) +
> > 			    I2C_PACKET_HEADER_SIZE;
> > 
> > 	dma = ((xfer_size > I2C_PIO_MODE_MAX_LEN) &&
> > 		i2c_dev->tx_dma_chan && i2c_dev->rx_dma_chan);
> > 
> > 	if (dma) {
> > 		buffer = kmalloc(ALIGN(msg->len, BYTES_PER_FIFO_WORD) +
> > 				I2C_PACKET_HEADER_SIZE, GFP_KERNEL);
> > 		i2c_dev->msg_buf = (u8 *)buffer;
> > 	}
> > 
> > 	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
> > 				PACKET_HEADER0_PROTOCOL_I2C |
> > 				(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
> > 				(1 << PACKET_HEADER0_PACKET_ID_SHIFT);
> > 		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> > 
> > 	if (dma)
> > 		(*buffer++) = packet_header;
> > 	else
> > 		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> > 
> > ... and so on.
> > 
> > Likely that kmalloc overhead will be bigger than the above variant. Please consider to avoid kmalloc and copying for the PIO case in the next version.
> > 
>
> Also, is the intermediate kmalloc "buffer" really needed at all? Why just not to write directly into the DMA buffer?

Yes, that’s what I am thinking too. Changing it. Will send revised patch..