linux-kernel - Re: [PATCHv5 1/2] gpio: sch: Consolidate similar algorithms

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] [day] [month] [year] [list]

Message-ID: <CAAVeFuJ+rus5+tjdYxUwGahNCSR1RnoFt=8XEHZ-jznvFj+L4g@mail.gmail.com>
Date:	Thu, 15 Jan 2015 19:11:48 +0900
From:	Alexandre Courbot <gnurou@...il.com>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Chang Rebecca Swee Fun <rebecca.swee.fun.chang@...el.com>,
	Mika Westerberg <mika.westerberg@...el.com>,
	GPIO Subsystem Mailing List <linux-gpio@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Denis Turischev <denis.turischev@...pulab.co.il>
Subject: Re: [PATCHv5 1/2] gpio: sch: Consolidate similar algorithms

On Thu, Jan 15, 2015 at 6:49 PM, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Mon, Dec 8, 2014 at 10:38 AM, Chang Rebecca Swee Fun
> <rebecca.swee.fun.chang@...el.com> wrote:
>
>> Consolidating similar algorithms into common functions to make
>> GPIO SCH simpler and manageable.
>>
>> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@...el.com>
>> Reviewed-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
>> Reviewed-by: Alexandre Courbot <acourbot@...dia.com>
>
> I have removed this patch from the tree. It breaks completely
> in build and looks strange:
>
>> +static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
>> +                            int val)
>
> Takes struct gpio_chip * as argument...
>
>> +static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
>> +{
>> +       struct sch_gpio *sch = to_sch_gpio(gc);
>>
>> +       spin_lock(&sch->lock);
>> +       sch_gpio_reg_set(sch, gpio_num, GIO, 1);
>
> Passes something else as argument.
>
>>  static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
>>  {
>>         struct sch_gpio *sch = to_sch_gpio(gc);
>> -       u8 curr_vals;
>> -       unsigned short offset, bit;
>>
>>         spin_lock(&sch->lock);
>> -
>> -       offset = sch_gpio_offset(sch, gpio_num, GLV);
>> -       bit = sch_gpio_bit(sch, gpio_num);
>> -
>> -       curr_vals = inb(sch->iobase + offset);
>> -
>> -       if (val)
>> -               outb(curr_vals | (1 << bit), sch->iobase + offset);
>> -       else
>> -               outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
>> -
>> +       sch_gpio_reg_set(gc, gpio_num, GLV, val);
>
> Here it is correct.
>
>> @@ -139,18 +123,9 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>>                                   int val)
>>  {
>>         struct sch_gpio *sch = to_sch_gpio(gc);
>> -       u8 curr_dirs;
>> -       unsigned short offset, bit;
>>
>>         spin_lock(&sch->lock);
>> -
>> -       offset = sch_gpio_offset(sch, gpio_num, GIO);
>> -       bit = sch_gpio_bit(sch, gpio_num);
>> -
>> -       curr_dirs = inb(sch->iobase + offset);
>> -       if (curr_dirs & (1 << bit))
>> -               outb(curr_dirs & ~(1 << bit), sch->iobase + offset);
>> -
>> +       sch_gpio_reg_set(sch, gpio_num, GIO, 0);
>
> Wrong again. Etc.
>
> Makes me suspect that this patch is not tested at all, so dropped.

IIRC at least one of the previous versions had the same issue and at
that time I already pointed out that *compiling and testing* patches
before sending them in the wild is common sense.

Ironically the cover letter says "The patches has been verifed and
tested working on Galileo Board". One may wonder...
--
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/