linux-kernel - Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

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: <4DC0E30A.1060308@gmail.com>
Date:	Wed, 04 May 2011 09:24:26 +0400
From:	Igor Plyatov <plyatov@...il.com>
To:	James Nuss <jamesnuss@...ometrics.ca>
CC:	Alexander Gordeev <lasaine@....cs.msu.su>,
	Rodolfo Giometti <rodolfo.giometti@...eenne.com>,
	linuxpps@...enneenne.com, linux-kernel@...r.kernel.org
Subject: Re: [LinuxPPS] [PATCH 2/2] pps: new client driver using IRQs

Hi James!

> Thanks for you comments Igor, Alexander and Rodolfo.
>
> You all make a good point that it would be useful to also register a
> CLEAR event. Before I go ahead and implement any changes I would like
> clarify a couple of things.
>
> My original patch which only included the ASSERT event was simple
> because all you needed to do was register an IRQ resource with a
> particular edge trigger and you would get an ASSERT event on that
> edge. It becomes a little more complicated when you want to capture
> both ASSERT and CLEAR events because you also have to indicate to the
> driver which edge is the ASSERT and which is the CLEAR. This is
> because I presume you could have systems that generate a pulse which
> transitions from low->high->low OR a pulse which transitions from
> high->low->high, and we would want a driver which is generic enough to
> handle both cases. Is my understanding correct that an ASSERT event is
> simply the first edge of a single pulse, regardless of "polarity"?
>
> I suppose the driver could be intelligent enough to figure out which
> is the first edge on the assumption that the pulse width is always
> very small - at least less than 0.5 second, more likely less than a
> few hundred milliseconds. Obviously it would need at least 2 pulses
> before it decides to register either an ASSERT or CLEAR event. Is it
> safe enough to say that an ASSERT event will only be generated if the
> previous edge was greater than 0.5 second ago, otherwise a CLEAR event
> will be generated?

This way can lead to problems when developer tries to debug PPS 
subsystem with some specific signal parameters.
In my opinion it much safer to explicitly declare ".active_low = 0" or 
".active_low = 1" in the platform initialization code.
See example:

/*
  * pps client gpio
  */
static struct pps_client_gpio pps_client_gpios[] = {
     {
         .gpio            = GPI_PPS0_IN,
         .active_low        = 0, /* ASSERT is a _/ */
         .desc            = "pps0 source",
         .decoder_type        = PPS_DECODER_GEOSIG_T1PPS,
     },
     {
         .gpio            = GPI_PPS1_IN,
         .active_low        = 1, /* ASSERT is a \_ */
         .desc            = "pps1 source",
     }
};

static struct pps_client_gpio_platform_data pps_client_gpios_data = {
     .pclient    = pps_client_gpios,
     .num_gpios    = ARRAY_SIZE(pps_client_gpios),
};

static struct platform_device pps_client_gpio_device = {
     .name        = "pps-client-gpio",
     .id        = 0,
     .dev        = {
         .platform_data    = &pps_client_gpios_data,
     }
};


> If I was to implement the driver this way then you would have exactly
> 3 ways to register a device to use this driver:
>
> 1) Register an IRQ with only IORESOURCE_IRQ_HIGHEDGE set:
> This will generate an ASSERT event on rising edges (no CLEAR events)
>
> 2) Register an IRQ with only IORESOURCE_IRQ_LOWEDGE set:
> This will generate an ASSERT event on falling edges (no CLEAR events)
>
> 3) Register an IRQ with both IORESOURCE_IRQ_LOWEDGE and
> IORESOURCE_IRQ_HIGHEDGE set:
> This will generate ASSERT and CLEAR events with the driver dynamically
> determining which edge is the ASSERT based on the logic above.
>
> I hope this covers all the potential use cases.
>
> cheers,
> James
>
>
> On Fri, Apr 29, 2011 at 4:26 AM, Rodolfo Giometti<giometti@...eenne.com>  wrote:
>> On Fri, Apr 29, 2011 at 08:29:55AM +0400, Igor Plyatov wrote:
>>
>>>>> The latter will definitely mess things up, right?
>>>> I mean, one surely can register an IRQ resource with both flags set. And
>>>> if the underlying hardware works as it is described (i.e. raises an irq
>>>> on both edges) then it will be a problem.
>>> Please don't try to abandon one of ASSERT or CLEAR events!
>>> It is very useful to register both of them, because in this case its
>>> possible to measure pulse width and decode PPS signals like DCF77.
>> At this point I suppose we should add both ASSERT and CLEAR events...
>>
>> Ciao,
>>
>> Rodolfo
>>
>> --
>>
>> GNU/Linux Solutions                  e-mail: giometti@...eenne.com
>> Linux Device Driver                          giometti@...ux.it
>> Embedded Systems                     phone:  +39 349 2432127
>> UNIX programming                     skype:  rodolfo.giometti
>> Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
>> --
>> 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/
>>

Best regards!

-- 
Igor Plyatov

--
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/