linux-kernel - Re: [PATCH v2 1/7] [media] rc-main: assign driver type during allocation

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <20160902052514.fmjk4mrhblztthzb@samsunx.samsung>
Date:   Fri, 02 Sep 2016 14:25:14 +0900
From:   Andi Shyti <andi.shyti@...sung.com>
To:     Sean Young <sean@...s.org>
Cc:     Mauro Carvalho Chehab <mchehab@....samsung.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Andi Shyti <andi@...zian.org>
Subject: Re: [PATCH v2 1/7] [media] rc-main: assign driver type during
 allocation

Hi Sean,

> >  	ir = kzalloc(sizeof(*ir), GFP_KERNEL);
> > -	dev = rc_allocate_device();
> > +	dev = rc_allocate_device(RC_DRIVER_IR_RAW);
> >  	if (!ir || !dev)
> >  		goto err_out_free;
> >  
> 
> If ir->sampling = 0 then it should be RC_DRIVER_SCANCODE.
> 
> 
> > @@ -481,7 +481,6 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci)
> >  	dev->scancode_mask = hardware_mask;
> >  
> >  	if (ir->sampling) {
> > -		dev->driver_type = RC_DRIVER_IR_RAW;
> >  		dev->timeout = 10 * 1000 * 1000; /* 10 ms */
> >  	} else {
> >  		dev->driver_type = RC_DRIVER_SCANCODE;
> 
> That assignment shouldn't really be there any more.

I think this doesn't change the driver's behavior, because I
either do like:

  -   dev = rc_allocate_device();
  +   dev = rc_allocate_device(RC_DRIVER_SCANCODE);

  [ ... ]

      if (ir->sampling) {
              dev->driver_type = RC_DRIVER_IR_RAW;
              dev->timeout = 10 * 1000 * 1000; /* 10 ms */
      } else {
   -          dev->driver_type = RC_DRIVER_SCANCODE;

Or I would need to do aftr the long switch...case statement

    +  if (ir->sampling) {
    +          dev = rc_allocate_device(RC_DRIVER_IR_RAW);
    +          ...
    +  } else {
    +          dev = rc_allocate_device(RC_DRIVER_SCANCODE);
    +          ...

I prefered the first way because it doesn't alter much the
driver.

> >  	ir = kzalloc(sizeof(*ir), GFP_KERNEL);
> > -	rc = rc_allocate_device();
> > +	rc = rc_allocate_device(RC_DRIVER_SCANCODE);
> >  	if (!ir || !rc) {
> >  		err = -ENOMEM;
> >  		goto err_out_free;
> 
> This is not correct, I'm afraid. If you look at the code you can see that
> if raw_decode is true, then it should be RC_DRIVER_IR_RAW.

Same here, the driver doesn't change the behavior. raw_decode can
be both 'true' or 'false' it's set as default RC_DRIVER_SCANCODE
and depending on value of raw_decode it's chaged to
RC_DRIVER_IR_RAW.

also in this case I can do

    +  if (raw_decode) {
    +          rc = rc_allocate_device(RC_DRIVER_IR_RAW);
    +          ...
    +  } else {
    +          rc = rc_allocate_device(RC_DRIVER_SCANCODE);
    +          ...

but also in this case my original approach doesn't add much
changes.

Thanks,
Andi