linux-kernel - Re: [RFC PATCH v2 3/4] drivers/auxdisplay: senshat Raspberry Pi Sense HAT display driver

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]

Message-ID: <b7e9dc19babef0e6993fa95be183c598a2f49bbd.camel@redhat.com>
Date:   Mon, 30 Aug 2021 15:28:51 +0200
From:   nsaenzju@...hat.com
To:     Charles Mirabile <cmirabil@...hat.com>,
        linux-kernel@...r.kernel.org
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Serge Schneider <serge@...pberrypi.org>,
        Stefan Wahren <stefan.wahren@...e.com>,
        linux-rpi-kernel@...ts.infradead.org, fedora-rpi@...glegroups.com,
        Mwesigwa Guma <mguma@...hat.com>,
        Joel Savitz <jsavitz@...hat.com>
Subject: Re: [RFC PATCH v2 3/4] drivers/auxdisplay: senshat Raspberry Pi
 Sense HAT display driver

Hi Charles,

On Fri, 2021-08-20 at 14:08 -0400, Charles Mirabile wrote:
> This patch implements control of the 8x8 RGB LED matrix display.

It'd be nice to get a more information on the i2c interface, and what each byte
is supposed to represent.

> Signed-off-by: Charles Mirabile <cmirabil@...hat.com>
> Signed-off-by: Mwesigwa Guma <mguma@...hat.com>
> Signed-off-by: Joel Savitz <jsavitz@...hat.com>
> ---

[...]

> +static long sensehat_display_ioctl(struct file *filp, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	struct sensehat *sensehat = container_of(filp->private_data, struct sensehat, display.mdev);
> +	struct sensehat_display *sensehat_display = &sensehat->display;
> +	void __user *user_ptr = (void __user *)arg;
> +	u8 temp[GAMMA_SIZE];
> +	int ret;
> +
> +	if (mutex_lock_interruptible(&sensehat_display->rw_mtx))
> +		return -ERESTARTSYS;
> +	switch (cmd) {
> +	case SENSEDISP_IOGET_GAMMA:
> +		if (copy_to_user(user_ptr, sensehat_display->gamma, GAMMA_SIZE)) {
> +			ret = -EFAULT;
> +			goto out_unlock;
> +		}
> +		ret = 0;
> +		goto out_unlock;
> +	case SENSEDISP_IOSET_GAMMA:
> +		if (copy_from_user(temp, user_ptr, GAMMA_SIZE)) {
> +			ret = -EFAULT;
> +			goto out_unlock;
> +		}
> +		ret = 0;
> +		goto out_update;
> +	case SENSEDISP_IORESET_GAMMA:
> +		if (arg < GAMMA_DEFAULT || arg >= GAMMA_PRESET_COUNT) {
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
> +		memcpy(temp, gamma_presets[arg], GAMMA_SIZE);
> +		ret = 0;
> +		goto out_update;
> +	default:
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +out_update:
> +	memcpy(sensehat_display->gamma, temp, GAMMA_SIZE);
> +	sensehat_update_display(sensehat);
> +out_unlock:
> +	mutex_unlock(&sensehat_display->rw_mtx);
> +	return ret;
> +}
> +
> +static const struct file_operations sensehat_display_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= sensehat_display_llseek,
> +	.read		= sensehat_display_read,
> +	.write		= sensehat_display_write,
> +	.unlocked_ioctl	= sensehat_display_ioctl,
> +};

I doubt this approach will make it upstream. This should use an already
existing kernel interface, or if not good enough, extend it. I'm sure this is
not the first RGB led matrix to show up anyway. Maybe drivers/leds has
infrastructure to deal with this. Or else a fb device?

I presume you want to keep the IOCTL in order to be able to run RPi specific
aplications/libraries. It'll be up to them to change once we settle on a proper
way of handling this, not the other way around. Note that the RPi engineers
have always preffered using official kernel interfaces when available, so I
don't think this'll be problematic.

Regards,
Nicolas