| 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
| ||
|
Message-ID: <4CC825D2.3030108@pengutronix.de>
Date: Wed, 27 Oct 2010 15:14:58 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Tomoya MORINAGA <tomoya-linux@....okisemi.com>
CC: Wolfgang Grandegger <wg@...ndegger.com>,
andrew.chih.howe.khor@...el.com, socketcan-core@...ts.berlios.de,
sameo@...ux.intel.com, margie.foster@...el.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
yong.y.wang@...el.com, masa-korg@....okisemi.com,
kok.howg.ewe@...el.com, chripell@...e.org,
morinaga526@....okisemi.com, David Miller <davem@...emloft.net>,
joel.clark@...el.com, qi.wang@...el.com
Subject: Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix buildwarnings
On 10/27/2010 01:27 PM, Tomoya MORINAGA wrote:
> On Wednesday, October 27, 2010 3:52 AM : Marc Kleine-Budde and Wolfgang Grandegge wrote:
>> Do I understand your code correctly? You have a big loop, but only do
>> two different things at certain values of the loop? Smells fishy.
> Uh, I can't understand your intention.
> Please show in detail.
It's easier to talk about code when we can see it, pelase don't delete :)
>> +static void pch_can_config_rx_tx_buffers(struct pch_can_priv *priv)
>> > +{
>> > + int i;
>> > + unsigned long flags;
>> > +
>> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>> > +
>> > + for (i = 0; i < PCH_OBJ_NUM; i++) {
>> > + if (priv->msg_obj[i] == MSG_OBJ_RX) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > + iowrite32(CAN_CMASK_RX_TX_GET,
>> > + &priv->regs->if1_cmask);
>> > + pch_can_check_if_busy(&priv->regs->if1_creq, i+1);
>> > +
>> > + iowrite32(0x0, &priv->regs->if1_id1);
>> > + iowrite32(0x0, &priv->regs->if1_id2);
>> > +
>> > + pch_can_bit_set(&priv->regs->if1_mcont,
>> > + CAN_IF_MCONT_UMASK);
>> > +
>> > + /* Set FIFO mode set to 0 except last Rx Obj*/
>> > + pch_can_bit_clear(&priv->regs->if1_mcont,
>> > + CAN_IF_MCONT_EOB);
>> > + /* In case FIFO mode, Last EoB of Rx Obj must be 1 */
>> > + if (i == (PCH_RX_OBJ_NUM - 1))
>> > + pch_can_bit_set(&priv->regs->if1_mcont,
>> > + CAN_IF_MCONT_EOB);
>> > +
>> > + iowrite32(0, &priv->regs->if1_mask1);
>> > + pch_can_bit_clear(&priv->regs->if1_mask2,
>> > + 0x1fff | CAN_MASK2_MDIR_MXTD);
>> > +
>> > + /* Setting CMASK for writing */
>> > + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK |
>> > + CAN_CMASK_ARB | CAN_CMASK_CTRL,
>> > + &priv->regs->if1_cmask);
>> > +
>> > + pch_can_check_if_busy(&priv->regs->if1_creq, i+1);
>> > + } else if (priv->msg_obj[i] == MSG_OBJ_TX) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Do I understand your code correctly? You have a big loop, but only do
> two different things at certain values of the loop? Smells fishy.
Looking again at the code it makes sense as it is :) Sorry for the
confusion.
>> > + iowrite32(CAN_CMASK_RX_TX_GET,
>> > + &priv->regs->if2_cmask);
>> > + pch_can_check_if_busy(&priv->regs->if2_creq, i+1);
>> > +
>> > + /* Resetting DIR bit for reception */
>> > + iowrite32(0x0, &priv->regs->if2_id1);
>> > + iowrite32(0x0, &priv->regs->if2_id2);
>> > + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID2_DIR);
>> > +
>> > + /* Setting EOB bit for transmitter */
>> > + iowrite32(CAN_IF_MCONT_EOB, &priv->regs->if2_mcont);
>> > +
>> > + pch_can_bit_set(&priv->regs->if2_mcont,
>> > + CAN_IF_MCONT_UMASK);
>> > +
>> > + iowrite32(0, &priv->regs->if2_mask1);
>> > + pch_can_bit_clear(&priv->regs->if2_mask2, 0x1fff);
>> > +
>> > + /* Setting CMASK for writing */
>> > + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK |
>> > + CAN_CMASK_ARB | CAN_CMASK_CTRL,
>> > + &priv->regs->if2_cmask);
>> > +
>> > + pch_can_check_if_busy(&priv->regs->if2_creq, i+1);
>> > + }
>> > + }
>> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>> > +}
> This processing does configuration for all message objects.
Yeah, got it. However I think you can get rid of the priv->msg_obj
variable altogether. Let me recapitulate:
- you setup priv->msg_obj[] in the probe function, which defines if a
msg_obj is a rx or tx
- this definition is never changed
- all objects of one kind are in a row
So you can identify the purpose of a msg_obj by simply looking at it's
number. If you need to loop over them you can even define helper
functions like, for_each_rx_obj().
>> what does this loop do? why is it nessecarry? I don't like delay loops
>> in the hot path of a driver.
> This loop is for waiting for all tx Message Object completion.
> This is Topcliff CAN HW specification.
Can you give us a pointer into intel's documentation?
I think Wolfgang already suggested to check if the chip is busy _before_
accessing it instead of waiting the chip to finish after accessing.
>> If you figured out how to use the endianess conversion functions from
>> the cpu_to_{le,be}-{le,to}_to_cpup family use them here, too.
> Uh,le32_to_cpu have been used already here.
Let's look at the code:
>> + for (i = 0, j = 0; i < cf->can_dlc; j++) {
>> > + reg = ioread32(&priv->regs->if1_dataa1 + j*4);
>> > + cf->data[i++] = cpu_to_le32(reg & 0xff);
>> > + if (i == cf->can_dlc)
>> > + break;
>> > + cf->data[i++] = cpu_to_le32((reg >> 8) & 0xff);
>> > + }
What does the code do? It swaps bytes because the data bytes in the can
core is arranged differently compared to the data in the struct can_frame.
According to the datasheet if_dataa1 holds 1st byte in bits 07:00 and
2nd byte in 15:08. (The rest is reserved.) So in the memory it looks
like this:
xx xx byte1 byte0
The can_frame has a different layout:
__u8 data[8] __attribute__((aligned(8)));
which is in memory:
byte0 byte1 byte2 byte3 byte4 byte5 byte6 byte7
This is why you swap. However in Linux no need to do this by hand.
The if_dataXX have a little endian layout, while the can frame has a big
endian layout. Further if_dataXX has only 16 bit of can data.
I think it should look like this:
for (i = 0; i < cf->can_dlc; i += 2) {
reg = ioread32(&priv->regs->if1_data[i >> 1]);
*(__be16 *)cf->data[i] = cpu_to_be16(reg);
}
You have to change the definition of the regs struct a bit:
> u32 if1_mcont;
> u32 if1_data[4];
> u32 reserve2;
Totally untested, though.
BTW: Where can I get this Intel Hardware to improve and test the driver?
> I can't understand your intention.
> Please show in detail.
Above we have the RX-Path, the TX-path would probably use a
"be16_to_cpup", have a look at the flexcan driver. It uses the whole 32
bit for candata, though.
>>> All these check if busy in the code make me a bit nervous, can you
>>> please explain why they are needed. A pointer to the manual is okay, too.
>> Me too. I already ask in my previous mail how long that functions
>> usually blocks.
> When accessing read/write from/to Message RAM,
> Since it takes much time for transferring between Register and Message RAM,
> SW must check busy flag of CAN register.
> This is a Topcliff HW specification.
see above.
>> is there some pdev->name instead of KBUILD_MODNAME that can be used?
> I can't understand your intention.
> pdev(struct pci_dev) doesn't have "name" member.
I was just asking :) As it doesn't have a name, KBUILD_MODNAME is fine.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Download attachment "signature.asc" of type "application/pgp-signature" (263 bytes)
Powered by blists - more mailing lists