linux-kernel - Re: [PATCH] Make appletouch shut up when it has nothing to say

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: <20070513114626.d943999a.zaitcev@redhat.com>
Date:	Sun, 13 May 2007 11:46:26 -0700
From:	Pete Zaitcev <zaitcev@...hat.com>
To:	Matthew Garrett <mjg59@...f.ucam.org>
Cc:	Soeren Sonnenburg <kernel@....de>,
	linux-input@...ey.karlin.mff.cuni.cz, nicolas@...chat.ch,
	linux-usb-devel@...ts.sourceforge.net,
	Linux Kernel <linux-kernel@...r.kernel.org>, zaitcev@...hat.com
Subject: Re: [PATCH] Make appletouch shut up when it has nothing to say

On Sun, 13 May 2007 18:20:53 +0100, Matthew Garrett <mjg59@...f.ucam.org> wrote:

> +static void atp_reinit(struct work_struct *work)
> +{
> +	char data[8];
> +	size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> +			       ATP_GEYSER3_MODE_READ_REQUEST_ID,
> +			       USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +			       ATP_GEYSER3_MODE_REQUEST_VALUE,
> +			       ATP_GEYSER3_MODE_REQUEST_INDEX, &data, 8, 5000);

Generally speaking, DMA-ing from stack is not allowed. Maybe you want
to add a comment saying "buffer on stack because this is Mac-only driver".
I would not ask for kmalloc here which is entirely pointless.

> +
> +		/* Geyser 3 will continue to send packets continually after
> +		   the first touch unless reinitialised. Do so if it's been
> +		   idle for a while in order to avoid waking the kernel up
> +		   several hundred times a second */
> +		if (dev->idlecount >= 10)
> +			queue_work (appletouch_wq, &dev->work);

So, why don't you check for Geyser 3 or whatnot here instead of doing
it inside the work function? Why poking workqueue on other systems?

> +	appletouch_wq = create_singlethread_workqueue("kappletouch");
> +	if (!appletouch_wq) {
> +		printk(KERN_ERR "appletouch: failed to create workqueue\n");
> +		return -ENOMEM;
> +	}
>  	return usb_register(&atp_driver);

I'm wondering about the wisdom of this too. Why won't the stock
workqueue work? We have way too many daemons already. Plus, making
one more unconditionally regardless of hardware present seems like
stepping over the line to me.

Overall this seems like a good patch to me, only needs to be less
intrusive for owners of other hardware.

-- Pete
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/