linux-kernel - Re: [PATCH v4 02/10] pinctrl: mvebu: dove pinctrl driver

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: <51C04C23.4090807@gmail.com>
Date:	Tue, 18 Jun 2013 14:01:39 +0200
From:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
CC:	Lior Amsalem <alior@...vell.com>,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Rob Herring <rob.herring@...xeda.com>,
	Ben Dooks <ben.dooks@...ethink.co.uk>,
	devicetree-discuss@...ts.ozlabs.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 02/10] pinctrl: mvebu: dove pinctrl driver

On 06/18/13 13:36, Russell King - ARM Linux wrote:
> On Thu, Sep 13, 2012 at 05:41:44PM +0200, Sebastian Hesselbarth wrote:
>> +#define DOVE_GLOBAL_CONFIG_1		(DOVE_SB_REGS_VIRT_BASE | 0xe802C)
>> +#define  DOVE_TWSI_ENABLE_OPTION1	BIT(7)
>> +#define DOVE_GLOBAL_CONFIG_2		(DOVE_SB_REGS_VIRT_BASE | 0xe8030)
>> +#define  DOVE_TWSI_ENABLE_OPTION2	BIT(20)
>> +#define  DOVE_TWSI_ENABLE_OPTION3	BIT(21)
>> +#define  DOVE_TWSI_OPTION3_GPIO		BIT(22)
> ...

Russell,

the above absolute addresses already made me think of cleaning up dove
pinctrl a while ago. I also had in mind that below function exclusively
request ownership of global config registers.

>> +static int dove_twsi_ctrl_set(struct mvebu_mpp_ctrl *ctrl,
>> +				unsigned long config)
>> +{
>> +	unsigned long gcfg1 = readl(DOVE_GLOBAL_CONFIG_1);
>> +	unsigned long gcfg2 = readl(DOVE_GLOBAL_CONFIG_2);
>> +
>> +	gcfg1 &= ~DOVE_TWSI_ENABLE_OPTION1;
>> +	gcfg2 &= ~(DOVE_TWSI_ENABLE_OPTION2 | DOVE_TWSI_ENABLE_OPTION2);
>> +
>> +	switch (config) {
>> +	case 1:
>> +		gcfg1 |= DOVE_TWSI_ENABLE_OPTION1;
>> +		break;
>> +	case 2:
>> +		gcfg2 |= DOVE_TWSI_ENABLE_OPTION2;
>> +		break;
>> +	case 3:
>> +		gcfg2 |= DOVE_TWSI_ENABLE_OPTION3;
>> +		break;
>> +	}
>> +
>> +	writel(gcfg1, DOVE_GLOBAL_CONFIG_1);
>> +	writel(gcfg2, DOVE_GLOBAL_CONFIG_2);
>> +
>> +	return 0;
>> +}
>
> So, I've just been thinking about the LCD clocking on the Armada 510,
> and found that there's dividers for the internal LCD clocks in the
> global config 1/2 registers.  So I grepped the kernel source for
> references to these, expecting to find something in drivers/clk, but
> found the above.

We have no peripheral clock handling for Dove, yet. Just core clocks and
clock gates are implemented. And I guess they are DT only anyway.

> However, todays kernel is sometimes SMP, commonly with kernel preemption
> enabled, maybe even RT.  This makes things like the above sequence a
> problem where a multifunction register is read, modified and then
> written back.
>
> Consider two threads doing this, and a preemption event happening in the
> middle of this sequence to another thread also doing a read-modify-write
> of the same register.  Which one wins depends on the preemption sequence,
> but ultimately one loses out.

Yeah, sure. We have the same issue with watchdog driver messing with
timer registers. There I exported a function to _clrset TIMER_CTRL
register safely. Just went into irqchip (tip for-next).

> Any access to such registers needs careful thought, and protection in some
> manner.
>
> Maybe what we need is something like this:
>
> static DEFINE_SPINLOCK(io_lock);
> static void modifyl(u32 new, u32 mask, void __iomem *reg)
> {
> 	unsigned long flags;
> 	u32 val;
>
> 	spin_lock_irqsave(&io_lock, flags);
> 	val = readl(reg) & ~mask;
> 	val |= new | mask;
> 	writel(val, reg);
> 	spin_unlock_irqrestore(&io_lock, flags);
> }
>
> in order to provide arbitrated access to these kinds of multifunction
> registers in a safe, platform agnostic way.

I am fine with a generic modify function with a single lock. Most cases
should be fine with a single lock even for non-related register
accesses, e.g. watchdog will access TIMER_CTRL only once to enable
itself. If you think you need a special lock because you have a lot of
writes to shared registers, you can still have your own modify lock.

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