linux-kernel - RE: [PATCH-V2 1/3] RTC:s35390a: Add Alarm interrupt support

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]

Date:	Mon, 23 Aug 2010 10:28:42 +0530
From:	"Hiremath, Vaibhav" <hvaibhav@...com>
To:	"felipe.balbi@...ia.com" <felipe.balbi@...ia.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"byron.bbradley@...il.com" <byron.bbradley@...il.com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>
Subject: RE: [PATCH-V2 1/3] RTC:s35390a: Add Alarm interrupt support

> -----Original Message-----
> From: Felipe Balbi [mailto:felipe.balbi@...ia.com]
> Sent: Saturday, August 21, 2010 7:36 PM
> To: Hiremath, Vaibhav
> Cc: linux-kernel@...r.kernel.org; akpm@...ux-foundation.org;
> byron.bbradley@...il.com; linux-omap@...r.kernel.org; Balbi Felipe (Nokia-
> MS/Helsinki)
> Subject: Re: [PATCH-V2 1/3] RTC:s35390a: Add Alarm interrupt support
> 
> Hi,
> 
> On Sat, Aug 21, 2010 at 02:30:27PM +0200, ext hvaibhav@...com wrote:
> >+static irqreturn_t s35390a_irq_thread(int irq, void *handle)
> >+{
> >+	char buf[1];
> >+	struct s35390a *s35390a = handle;
> >+	struct i2c_client *client = s35390a->client[0];
> 
> don't you need some locking on the irq handler ? a mutex maybe ? Just
> wondering...
> 
[Hiremath, Vaibhav] Yes definitely we do need locking here, I thought of adding locking mechanism in subsequent patch, does it makes sense?


> >@@ -261,15 +424,30 @@ static int s35390a_probe(struct i2c_client *client,
> > 	if (s35390a_get_datetime(client, &tm) < 0)
> > 		dev_warn(&client->dev, "clock needs to be set\n");
> >
> >+	if (client->irq >= 0) {
> >+		err = request_threaded_irq(client->irq, NULL,
> >+				s35390a_irq_thread,
> >+				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> >+				client->name, s35390a);
> >+		if (err) {
> >+			dev_err(&client->dev, "unable to request IRQ\n");
> >+			goto exit_dummy;
> >+		}
> >+	}
> >+
> > 	s35390a->rtc = rtc_device_register(s35390a_driver.driver.name,
> > 				&client->dev, &s35390a_rtc_ops, THIS_MODULE);
> >
> > 	if (IS_ERR(s35390a->rtc)) {
> > 		err = PTR_ERR(s35390a->rtc);
> >-		goto exit_dummy;
> >+		goto exit_intr;
> > 	}
> >+
> > 	return 0;
> >
> >+exit_intr:
> >+	free_irq(client->irq, client);
> 
> free_irq() won't behave correctly, I believe since you're passing
> different dev_id parameters. If you look at the implementation of
> free_irq() you'll see it uses dev_id to find the correct struct
> irqaction pointer.
> 
[Hiremath, Vaibhav] Overlooked, my bad. Thanks for pointing this to me ,will fix.


Thanks,
Vaibhav
> --
> balbi
> 
> DefectiveByDesign.org
--
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/