Hello Dan,
On Thursday, June 29, 2017, Dan Carpenter wrote:
>
> Hello Chris Brandt,
>
> The patch 8185e51f358a: "mmc: tmio-mmc: add support for 32bit data
> port" from Sep 12, 2016, leads to the following static checker
> warning:
>
> drivers/mmc/host/tmio_mmc_core.c:415 tmio_mmc_transfer_data()
> warn: passing casted pointer 'buf' to 'sd_ctrl_read32_rep()' 16 vs
> 32.
>
> drivers/mmc/host/tmio_mmc_core.c
> 401 static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
> 402 unsigned short *buf,
> 403 unsigned int count)
> 404 {
> 405 int is_read = host->data->flags & MMC_DATA_READ;
> 406 u8 *buf8;
> 407
> 408 /*
> 409 * Transfer the data
> 410 */
> 411 if (host->pdata->flags & TMIO_MMC_32BIT_DATA_PORT) {
> 412 u8 data[4] = { };
> 413
> 414 if (is_read)
> 415 sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT,
> (u32 *)buf,
>
> ^^^^^^^^^^
> 416 count >> 2);
>
> Of course, it's not invalid to cast pointer parameters, but it's often
> a sign that something is buggy. In the caller function we take a
> buffer of bytes, and we cast it to u16, and then here we cast it to u32.
> count is in terms of bytes still.
Personally, I think a buffer of raw data should be a 'u8 *', not a
'unsigned short *'.
And, the parameter name "count" should really be something like "size"
or "length" (but that is still confusing because the passed array 'buf'
was designated as an array of "unsigned short *", so what would 'size'
really mean??)
Anyway, that is the API that's in place today, so that's what I needed
to work with.
>
> 417 else
> 418 sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT,
> (u32 *)buf,
> 419 count >> 2);
> 420
> 421 /* if count was multiple of 4 */
> 422 if (!(count & 0x3))
> 423 return;
> 424
> 425 buf8 = (u8 *)(buf + (count >> 2));
>
>
> And this looks like the bug... buf is a u16 pointer but we're adding
> "count / sizeof(u32)" when we should be adding "count / (sizeof(*buf))".
I agree this math does not work out to what I wanted because of the
differences in data types.
Since I simply wanted to get a pointer to where I left off in the
buffer, maybe the most clear thing to do is just cast buf and do simple math:
buf8 = (u8 *)buf + (count & ~3);
Do you agree?
> 426 count %= 4;
> 427
> 428 if (is_read) {
> 429 sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT,
> 430 (u32 *)data, 1);
> 431 memcpy(buf8, data, count);
> 432 } else {
> 433 memcpy(data, buf8, count);
> 434 sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT,
> 435 (u32 *)data, 1);
> 436 }
> 437
> 438 return;
> 439 }
> 440
> 441 if (is_read)
> 442 sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf,
> count >> 1);
> 443 else
> 444 sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf,
> count >> 1);
> 445
> 446 /* if count was even number */
> 447 if (!(count & 0x1))
> 448 return;
> 449
> 450 /* if count was odd number */
> 451 buf8 = (u8 *)(buf + (count >> 1));
> 452
> 453 /*
> 454 * FIXME
> 455 *
> 456 * driver and this function are assuming that
> 457 * it is used as little endian
>
> Indeed.
>
> 458 */
> 459 if (is_read)
> 460 *buf8 = sd_ctrl_read16(host, CTL_SD_DATA_PORT) &
> 0xff;
> 461 else
> 462 sd_ctrl_write16(host, CTL_SD_DATA_PORT, *buf8);
> 463 }
>
> regards,
> dan carpenter
Chris
