Date: Mon, 18 Jun 2007 13:06:59 +0200 From: Attilio Rao <attilio@FreeBSD.org> To: Rui Paulo <rpaulo@fnop.net> Cc: Perforce Change Reviews <perforce@freebsd.org>, Rui Paulo <rpaulo@freebsd.org>, John Baldwin <jhb@freebsd.org> Subject: Re: PERFORCE change 120788 for review Message-ID: <46766753.3080807@FreeBSD.org> In-Reply-To: <86lkejcn8k.wl%rpaulo@fnop.net> References: <200706021756.l52Huq9A049371@repoman.freebsd.org> <86myzeq67f.wl%rpaulo@fnop.net> <4666B730.9080908@FreeBSD.org> <200706081351.54281.jhb@freebsd.org> <3bbf2fe10706081100k4f1457f2g6a714d8c897dc395@mail.gmail.com> <86lkejcn8k.wl%rpaulo@fnop.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Rui Paulo wrote:
> At Fri, 8 Jun 2007 20:00:18 +0200,
> Attilio Rao wrote:
>> 2007/6/8, John Baldwin <jhb@freebsd.org>:
>>> On Wednesday 06 June 2007 09:31:28 am Attilio Rao wrote:
>>>> Rui Paulo wrote:
>>>>> If I'm not doing something wrong, I need to use spin locks on my
>>>>> interrupt handler, or else witness_checkorder will complain with
>>>>> "blockable sleep lock".
>>>>>
>>>>> Note that I'm using FILTERs.
>>>> So you are doing this in the wrong way.
>>>> In order to use correctly filters, please note that the support for them
>>>> is compile time choosen, so you need to wrapper all filter specific
>>>> parts using INTR_FILTER compat macro.
>>> Actually, if you only use a filter and not an ithread handler, you can do that
>>> now w/o needing to have any #ifdef INTR_FILTER stuff.
>> In the case your kernel doesn't use filters (!INTR_FILTER) and you
>> pass a filter, it is automatically mapped to work as a fast handler?
>
> Ok, I've looked at sio(4) to see how it was setting up a fast intr
> handler.
>
> Does the following diff look correct? Thanks.
>
> ==== //depot/projects/soc2007/rpaulo-macbook/dev/asmc/asmc.c#13 - /home/rpaulo/p4/rpaulo-macbook/dev/asmc/asmc.c ====
> --- /tmp/tmp.96695.67 2007-06-17 03:44:50.000000000 +0100
> +++ /home/rpaulo/p4/rpaulo-macbook/dev/asmc/asmc.c 2007-06-17 03:44:17.000000000 +0100
> @@ -77,11 +77,8 @@ static int asmc_fan_getvalue(device_t, c
> static int asmc_temp_getvalue(device_t, const char *);
> static int asmc_sms_read(device_t, const char *, int16_t *);
> static void asmc_sms_calibrate(device_t);
> -#ifdef INTR_FILTER
> static int asmc_sms_intr(void *);
> -#else
> static void asmc_sms_fastintr(void *);
> -#endif
> static void asmc_sms_printintr(device_t, uint8_t);
>
> /*
> @@ -236,7 +233,7 @@ static int
> asmc_attach(device_t dev)
> {
> int i, j;
> - int error;
> + int ret;
> char name[2];
> struct asmc_softc *sc = device_get_softc(dev);
> struct asmc_model *model;
> @@ -378,16 +375,21 @@ asmc_attach(device_t dev)
> goto out;
> }
>
> -#ifdef INTR_FILTER
> - error = bus_setup_intr(dev, sc->sc_res,
> - INTR_TYPE_MISC | INTR_MPSAFE,
> - asmc_sms_intr, NULL, dev, &sc->sc_cookie);
> -#else
> - error = bus_setup_intr(dev, sc->sc_res,
> - INTR_TYPE_MISC | INTR_MPSAFE | INTR_FAST,
> - NULL, asmc_sms_fastintr, dev, &sc->sc_cookie);
> -#endif
> - if (error) {
> + ret = bus_setup_intr(dev, sc->sc_res,
> + INTR_TYPE_MISC | INTR_MPSAFE,
> + asmc_sms_intr, NULL, dev, &sc->sc_cookie);
> +
> + if (ret) {
> + ret = bus_setup_intr(dev, sc->sc_res,
> + INTR_TYPE_MISC | INTR_MPSAFE,
> + NULL, asmc_sms_fastintr, dev,
> + &sc->sc_cookie);
> + if (ret == 0)
> + device_printf(dev, "unable to setup fast interrupt. "
> + "Using normal mode.\n");
> + }
> +
> + if (ret) {
> device_printf(dev, "unable to setup SMS IRQ\n");
> bus_release_resource(dev, SYS_RES_IRQ, sc->sc_rid,
> sc->sc_res);
Generally, I don't like much the if (ret == 0) device_printf() since you
are doing a probe-by-try and so it should not display in this way IMHO.
Maybe having it under some debugging mechanism (e.g. KTR and similar)
would be acceptable.
My 2 cents.
Attilio
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?46766753.3080807>