From: Baruch Siach <baruch@tkos.co.il>
To: Indan Zupancic <indan@nul.nu>
Cc: Jiri Slaby <jslaby@suse.cz>,
linux-kernel@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
Greg KH <greg@kroah.com>, "H. Peter Anvin" <hpa@zytor.com>,
Alex Gershgorin <agersh@rambler.ru>
Subject: Re: [PATCHv3] drivers/misc: Altera active serial implementation
Date: Tue, 16 Nov 2010 07:56:02 +0200 [thread overview]
Message-ID: <20101116055602.GA18210@jasper.tkos.co.il> (raw)
In-Reply-To: <c30e46c1b5343789b6475722071c3020.squirrel@webmail.greenhost.nl>
Hi Indan,
On Tue, Nov 16, 2010 at 01:47:52AM +0100, Indan Zupancic wrote:
> On Mon, November 15, 2010 11:28, Jiri Slaby wrote:
> > On 11/15/2010 11:08 AM, Baruch Siach wrote:
> >> Hi Jiri,
> >>
> >> Thanks for reviewing. See my response to your comments below.
> >>
> >> On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
> >>> On 11/15/2010 09:23 AM, Baruch Siach wrote:
> >>>> --- /dev/null
> >>>> +++ b/drivers/misc/altera_as.c
> >>>> @@ -0,0 +1,349 @@
> >>> ...
> >>>> +struct altera_as_device {
> >>>> + unsigned id;
> >>>> + struct device *dev;
> >>>> + struct miscdevice miscdev;
> >>>> + struct mutex open_lock;
> >>>> + struct gpio gpios[AS_GPIO_NUM];
> >>>> +};
> >>>> +
> >>>> +/*
> >>>> + * The only functions updating this array are .probe/.remove, which are
> >>>> + * serialized. Hence, no locking is needed here.
> >>>> + */
> >>>> +static struct {
> >>>> + int minor;
> >>>
> >>> Why you need minor here? It's in drvdata->miscdev.minor.
> >>
> >> I use the minor field to lookup the drvdata pointer in get_as_dev(), which I
> >> use is altera_as_open(). I do this because I have no access to the 'struct
> >> device' pointer from the .open method. Do you know a better way?
>
> Yes.
>
> Get rid of the array and minor lookup, and instead use container_of()
> to get struct altera_as_device directly from file->private_data, which
> is a pointer to miscdev, set by misc_open().
Thanks. This was exactly what I was looking for.
> Sorry for not noticing this before.
>
> >>
> >>>> + struct altera_as_device *drvdata;
> >>>> +} altera_as_devs[AS_MAX_DEVS];
> >>>
> >>> You don't need the struct at all.
> >>> static struct altera_as_device *drvdata[AS_MAX_DEVS];
> >>> should be enough.
> >>
> >> See above.
> >
> > The answer to you previous question is here. You can just have a global
> > array of struct altera_as_device.
> >
> >>>> +static int altera_as_open(struct inode *inode, struct file *file)
> >>>> +{
> >>>> + int ret;
> >>>> + struct altera_as_device *drvdata = get_as_dev(iminor(inode));
> >>>> +
> >>>> + if (drvdata == NULL)
> >>>> + return -ENODEV;
> >>>> + file->private_data = drvdata;
> >>>> +
> >>>> + ret = mutex_trylock(&drvdata->open_lock);
> >>>> + if (ret == 0)
> >>>> + return -EBUSY;
> >>>> +
> >>>> + ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
> >>>> + if (ret < 0) {
> >>>> + mutex_unlock(&drvdata->open_lock);
> >>>> + return ret;
> >>>> + }
> >>>
> >>> So leaving to userspace with mutex held. This is *wrong*. use
> >>> test_and_set_bit or alike instead.
> >>
> >> Can you explain why this is wrong? I don't mind doing test_and_set_bit
> >> instead, I'm just curious.
> >
> > The mutex has an owner which it expects to unlock it. For example if you
> > fork a process which already opened the device, you have a problem. This
> > is a technical point. Another point is that it's ugly to leave to
> > userspace with any lock.
> >
> >>>> +static int __init altera_as_probe(struct platform_device *pdev)
> >>>> +{
> >>> ...
> >>>> + drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> >>>> + drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
> >>>> + pdata->id);
> >>>> + if (drvdata->miscdev.name == NULL)
> >>>> + return -ENOMEM;
> >>>> + drvdata->miscdev.fops = &altera_as_fops;
> >>>> + if (misc_register(&drvdata->miscdev) < 0) {
> >>>
> >>> Hmm, how many devices can be in the system?
> >>
> >> Up to AS_MAX_DEVS, currently 16.
> >>
> >>> This doesn't look like the right way.
> >>
> >> What is the right way then?
> >
> > Ok, so for that count it definitely deserves its own major to not eat up
> > misc device space.
>
> Ther are only as many minors allocates as needed, not always 16.
> Almost always there will be only one FPGA, maybe a couple, but
> more is very rare. And when that's the case there are probably
> not many other misc devices around either, or they use a more
> scalable way to program many FPGAs.
>
> This is niche enough, don't make it more complicated than needed.
> Having its own major seems overkill.
I agree.
> >>>> + kfree(drvdata->miscdev.name);
> >>>> + return -ENODEV;
> >>>> + }
> >>>> + altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
> >>>> + altera_as_devs[pdata->id].drvdata = drvdata;
> >>>
> >>> So now the device is usable without the mutex and gpios inited?
> >>
> >> I was thinking that the device lock which is being held during the .probe
> >> run,
> >> prevents the device from being open. After all I can still return -EGOAWAY
> >> at
> >> this point. Am I wrong?
> >>
> >> If so, I'll reorder this code.
> >
> > This cannot be done easily. You need to set drvdata prior to minor and
> > after all the assignments here. The former because in get_as_dev you
> > test minor and return drvdata. The latter because you use open_lock &
> > gpios after playing with minor & drvdata.
> >
> > Technically, it can be done with a use of barriers, but I don't
> > recommend it as drivers should not use barriers at all. You should
> > introduce some locking here (it introduces barriers on its own).
> >
> > So move the assignment to altera_as_devs below the mutex_init & gpios
> > setup and lock it appropriately. Then add a lock to get_as_dev.
>
> Just put the misc_register() call at the end and it should be fine,
> once the array is gone.
Thanks again. You were immensely helpful.
Going to remove some lines of code. I love the sound it makes.
baruch
> >>>> + mutex_init(&drvdata->open_lock);
> >>>> +
> >>>> + drvdata->id = pdata->id;
> >>>> +
> >>>> + drvdata->gpios[ASIO_DATA].gpio = pdata->data;
> >>>> + drvdata->gpios[ASIO_DATA].flags = GPIOF_IN;
> >>>> + drvdata->gpios[ASIO_DATA].label = "as_data";
> >>> ...
> >>>> + drvdata->gpios[ASIO_NCE].gpio = pdata->nce;
> >>>> + drvdata->gpios[ASIO_NCE].flags = GPIOF_OUT_INIT_HIGH;
> >>>> + drvdata->gpios[ASIO_NCE].label = "as_nce";
> >>>> +
> >>>> + dev_info(drvdata->dev, "Altera Cyclone Active Serial driver\n");
> >>>> +
> >>>> + return 0;
> >>>> +}
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
next prev parent reply other threads:[~2010-11-16 5:57 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-15 8:23 [PATCHv3] drivers/misc: Altera active serial implementation Baruch Siach
2010-11-15 9:33 ` Jiri Slaby
2010-11-15 10:08 ` Baruch Siach
2010-11-15 10:28 ` Jiri Slaby
2010-11-15 13:40 ` Baruch Siach
2010-11-15 14:24 ` Jiri Slaby
2010-11-15 16:19 ` Greg KH
2010-11-16 0:47 ` Indan Zupancic
2010-11-16 5:56 ` Baruch Siach [this message]
2010-11-16 7:42 ` H. Peter Anvin
2010-11-16 11:04 ` Alan Cox
2010-11-16 13:09 ` Jiri Slaby
2010-11-17 5:32 ` Baruch Siach
2010-11-17 5:43 ` H. Peter Anvin
2010-11-17 5:48 ` Baruch Siach
2010-11-17 8:31 ` Jiri Slaby
2010-11-17 11:08 ` Alan Cox
2010-11-16 11:05 ` Alan Cox
2010-11-17 6:18 ` Thomas Chou
2010-11-17 13:28 ` Baruch Siach
[not found] ` <4CE42457.8090001@zytor.com>
2010-11-18 2:15 ` Thomas Chou
2010-11-18 2:15 ` H. Peter Anvin
2010-11-22 5:57 ` Baruch Siach
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20101116055602.GA18210@jasper.tkos.co.il \
--to=baruch@tkos.co.il \
--cc=agersh@rambler.ru \
--cc=akpm@linux-foundation.org \
--cc=greg@kroah.com \
--cc=hpa@zytor.com \
--cc=indan@nul.nu \
--cc=jslaby@suse.cz \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).