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
| ||
|
Date: Sun, 12 Feb 2017 16:33:04 +0100 From: Noralf Trønnes <noralf@...nnes.org> To: Andy Shevchenko <andy.shevchenko@...il.com> Cc: dri-devel@...ts.freedesktop.org, devicetree <devicetree@...r.kernel.org>, robh@...nel.org, Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v4 2/7] drm/tinydrm: Add helper functions Den 12.02.2017 12.50, skrev Andy Shevchenko: > On Sat, Feb 11, 2017 at 8:48 PM, Noralf Trønnes <noralf@...nnes.org> wrote: >> Add common functionality needed by many tinydrm drivers. >> +int tinydrm_enable_backlight(struct backlight_device *backlight) >> +{ >> + unsigned int old_state; >> + int ret; >> + >> + if (!backlight) >> + return 0; >> + >> + old_state = backlight->props.state; >> + backlight->props.state &= ~BL_CORE_FBBLANK; >> + DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state, >> + backlight->props.state); > "%#x" ? > (And elsewhere) > >> +#if IS_ENABLED(CONFIG_SPI) >> +size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len) >> +{ >> + size_t ret; >> + >> + ret = min(spi_max_transfer_size(spi), spi->master->max_dma_len); > I don't get why DMA constrain somehow affects this framework? The reason is that spi-bcm2835 reverts to PIO on larger buffers. Looking at __spi_map_msg() and spi_map_buf() it becomes clear that the core breaks up the buffer into manageable parts. So this must be a bug in spi-bcm2835 (and spi-pxa2xx) since no other drivers have a upper limit in the .can_dma() callback. So you're rightly confused, with drivers fixed, this limit can be lifted. > What if max_dma_len is zero (imagine SPI master that works only by PIO > by some reason)? It can't be zero: int spi_register_master(struct spi_master *master) { ... if (!master->max_dma_len) master->max_dma_len = INT_MAX; >> + if (max_len) >> + ret = min(ret, max_len); >> + if (spi_max) >> + ret = min_t(size_t, ret, spi_max); >> + ret &= ~0x3; > Why alignment is that? Why do we need it? Isn't a busyness of SPI > framework to cope with it? This minimum capping doesn't look good. I should probably put >16 instead, that would cover the minimum for the 9-bit emulation code. The reason I let the user change the transfer size, is that some usb audio card on Raspberry Pi stuttered with 4k fbtft transfers. >> + if (ret < 4) > It's effectively check for 0. > >> + ret = 4; >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(tinydrm_spi_max_transfer_size); >> +static void >> +tinydrm_dbg_spi_print(struct spi_device *spi, struct spi_transfer *tr, >> + const void *buf, int idx, bool tx) >> +{ >> + u32 speed_hz = tr->speed_hz ? tr->speed_hz : spi->max_speed_hz; >> + char linebuf[3 * 32]; >> + >> + hex_dump_to_buffer(buf, tr->len, 16, >> + DIV_ROUND_UP(tr->bits_per_word, 8), >> + linebuf, sizeof(linebuf), false); >> + >> + printk(KERN_DEBUG >> + " tr(%i): speed=%u%s, bpw=%i, len=%u, %s_buf=[%s%s]\n", idx, >> + speed_hz > 1000000 ? speed_hz / 1000000 : speed_hz / 1000, >> + speed_hz > 1000000 ? "MHz" : "kHz", tr->bits_per_word, tr->len, >> + tx ? "tx" : "rx", linebuf, tr->len > 16 ? " ..." : ""); > I hope at some point we will have some extension to print speeds, > sizes and so on based on algo in string_get_size(). > >> +} >> +int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, >> + struct spi_transfer *header, u8 bpw, const void *buf, >> + size_t len) >> +{ >> + struct spi_transfer tr = { >> + .bits_per_word = bpw, >> + .speed_hz = speed_hz, >> + }; >> + struct spi_message m; >> + u16 *swap_buf = NULL; >> + size_t max_chunk; >> + size_t chunk; >> + int ret = 0; >> + >> + if (WARN_ON_ONCE(bpw != 8 && bpw != 16)) >> + return -EINVAL; >> + >> + max_chunk = tinydrm_spi_max_transfer_size(spi, 0); >> + >> + if (drm_debug & DRM_UT_DRIVER) >> + pr_debug("[drm:%s] bpw=%u, max_chunk=%zu, transfers:\n", >> + __func__, bpw, max_chunk); > For all of your dev_dbg() / pr_debug() __func__ argument might be > redundant. Dynamic Debug may include this by request from user. > >> +/** >> + * tinydrm_machine_little_endian - Machine is little endian >> + * >> + * Returns: >> + * true if *defined(__LITTLE_ENDIAN)*, false otherwise >> + */ >> +static inline bool tinydrm_machine_little_endian(void) >> +{ >> +#if defined(__LITTLE_ENDIAN) >> + return true; >> +#else >> + return false; >> +#endif >> +} > Hmm... What is the typical code of a caller for this? If the bus can't do 16-bit natively, it will have to swap the bytes on little endian machines before transfer as 8-bit (Raspberry Pi can't do 16-bit SPI with it's DMA capable controller). So this function is to avoid #ifdef's elsewhere.
Powered by blists - more mailing lists