| 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: <CAAObsKBAQP3isZejezEFgEDrXFMFWvGEOhYLCxHP4cZxzzt-4g@mail.gmail.com>
Date: Fri, 5 Aug 2016 12:46:29 +0200
From: Tomeu Vizoso <tomeu.vizoso@...labora.com>
To: Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc: Jonathan Corbet <corbet@....net>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
Daniel Vetter <daniel.vetter@...el.com>,
Emil Velikov <emil.velikov@...labora.com>
Subject: Re: [PATCH v3 2/3] drm: Add API for capturing frame CRCs
On 3 August 2016 at 09:06, Ville Syrjälä <ville.syrjala@...ux.intel.com> wrote:
> On Fri, Jul 22, 2016 at 04:10:44PM +0200, Tomeu Vizoso wrote:
>> Adds files and directories to debugfs for controlling and reading frame
>> CRCs, per CRTC:
>>
>> dri/0/crtc-0/crc
>> dri/0/crtc-0/crc/control
>> dri/0/crtc-0/crc/data
>>
>> Drivers can implement the set_crc_source callback() in drm_crtc_funcs to
>> start and stop generating frame CRCs and can add entries to the output
>> by calling drm_crtc_add_crc_entry.
>>
>> v2:
>> - Lots of good fixes suggested by Thierry.
>> - Added documentation.
>> - Changed the debugfs layout.
>> - Moved to allocate the entries circular queue once when frame
>> generation gets enabled for the first time.
>> v3:
>> - Use the control file just to select the source, and start and stop
>> capture when the data file is opened and closed, respectively.
>> - Make variable the number of CRC values per entry, per source.
>> - Allocate entries queue each time we start capturing as now there
>> isn't a fixed number of CRC values per entry.
>> - Store the frame counter in the data file as a 8-digit hex number.
>> - For sources that cannot provide useful frame numbers, place
>> XXXXXXXX in the frame field.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
>> ---
...
>> +static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>> + size_t len, loff_t *offp)
>> +{
>> + struct seq_file *m = file->private_data;
>> + struct drm_crtc *crtc = m->private;
>> + struct drm_crtc_crc *crc = &crtc->crc;
>> + char *source;
>> +
>> + if (len == 0)
>> + return 0;
>> +
>> + if (len > PAGE_SIZE - 1) {
>> + DRM_DEBUG_KMS("Expected < %lu bytes into crtc crc control\n",
>> + PAGE_SIZE);
>> + return -E2BIG;
>> + }
>> +
>> + source = kmalloc(len + 1, GFP_KERNEL);
>> + if (!source)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(source, ubuf, len)) {
>> + kfree(source);
>> + return -EFAULT;
>> + }
>
> memdup_user_nul() ?
Good call.
>> +
>> + if (source[len - 1] == '\n')
>> + source[len - 1] = '\0';
>> + else
>> + source[len] = '\0';
>> +
>> + spin_lock_irq(&crc->lock);
>> +
>> + if (crc->opened) {
>> + kfree(source);
>> + return -EBUSY;
>> + }
>
> Why not just start the thing here?
For the sake of symmetry, as we are stopping when the data file is closed.
>> +static struct drm_crtc_crc_entry *crtc_get_crc_entry(struct drm_crtc_crc *crc,
>> + int index)
>> +{
>> + void *p = crc->entries;
>> + size_t entry_size = (sizeof(*crc->entries) +
>> + sizeof(*crc->entries[0].crcs) * crc->values_cnt);
>
> This computation is duplicated also in crtc_crc_open(). could use a
> common helper to do it.
>
> Shame the language doesn't have a way to deal with arrays of variable
> sized arrays in a nice way.
Ok.
>> +
>> + return p + entry_size * index;
>> +}
>> +
>> +#define MAX_LINE_LEN (8 + 9 * DRM_MAX_CRC_NR + 1 + 1)
>> +
>> +static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
>> + size_t count, loff_t *pos)
>> +{
>> + struct drm_crtc *crtc = filep->f_inode->i_private;
>> + struct drm_crtc_crc *crc = &crtc->crc;
>> + struct drm_crtc_crc_entry *entry;
>> + char buf[MAX_LINE_LEN];
>> + int ret, i;
>> +
>> + spin_lock_irq(&crc->lock);
>> +
>> + if (!crc->source) {
>> + spin_unlock_irq(&crc->lock);
>> + return 0;
>> + }
>> +
>> + /* Nothing to read? */
>> + while (crtc_crc_data_count(crc) == 0) {
>> + if (filep->f_flags & O_NONBLOCK) {
>> + spin_unlock_irq(&crc->lock);
>> + return -EAGAIN;
>> + }
>> +
>> + ret = wait_event_interruptible_lock_irq(crc->wq,
>> + crtc_crc_data_count(crc),
>> + crc->lock);
>> + if (ret) {
>> + spin_unlock_irq(&crc->lock);
>> + return ret;
>> + }
>> + }
>> +
>> + /* We know we have an entry to be read */
>> + entry = crtc_get_crc_entry(crc, crc->tail);
>> +
>> + /*
>> + * 1 frame field of 8 chars plus a number of CRC fields of 8
>> + * chars each, space separated and with a newline at the end.
>> + */
>> + if (count < 8 + 9 * crc->values_cnt + 1 + 1) {
>
> Just < MAX_LINE_LEN perhaps? Or could make a macro/function that takes
> crc->values_cnt or DRM_MAX_CRC_NR as an argument.
Sounds good, went with a macro.
>> + spin_unlock_irq(&crc->lock);
>> + return -EINVAL;
>> + }
>> +
>> + BUILD_BUG_ON_NOT_POWER_OF_2(DRM_CRC_ENTRIES_NR);
>> + crc->tail = (crc->tail + 1) & (DRM_CRC_ENTRIES_NR - 1);
>> +
>> + spin_unlock_irq(&crc->lock);
>> +
>> + if (entry->has_frame_counter)
>> + snprintf(buf, 9, "%08x", entry->frame);
>> + else
>> + snprintf(buf, 9, "XXXXXXXX");
>
> Should we add "0x" prefix to all these numbers to make it clear that
> they're in fact hex?
Sounds like a good idea to me.
>> +
>> + for (i = 0; i < crc->values_cnt; i++)
>> + snprintf(buf + strlen(buf), 10, " %08x", entry->crcs[i]);
>
> The 'n' in snprintf() here seems pointless. As does the strlen().
Good.
>> + snprintf(buf + strlen(buf), 2, "\n");
>> +
>> + if (copy_to_user(user_buf, buf, strlen(buf) + 1))
>> + return -EFAULT;
>> +
>> + return strlen(buf) + 1;
>
> More strlen()s that shouldn't be needed.
Ok.
Thanks!
Tomeu
Powered by blists - more mailing lists