linux-kernel - Re: [PATCH] media: dvb_dummy_fe.c: add members to dvb_dummy_fe

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

Message-ID: <20191202065047.3cffda7a@kernel.org>
Date:   Mon, 2 Dec 2019 06:50:47 +0100
From:   Mauro Carvalho Chehab <mchehab@...nel.org>
To:     "Daniel W. S. Almeida" <dwlsalmeida@...il.com>
Cc:     gregkh@...uxfoundation.org, rfontana@...hat.com,
        kstewart@...uxfoundation.org, tglx@...utronix.de,
        skhan@...uxfoundation.org,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: dvb_dummy_fe.c: add members to
 dvb_dummy_fe_state

Em Mon, 2 Dec 2019 01:49:38 -0300
"Daniel W. S. Almeida" <dwlsalmeida@...il.com> escreveu:

> Hi Mauro, thanks for checking out my work!
> 
> > please don't add fields at the
> > struct while they're not used, as this makes harder for reviewers to be
> > sure that we're not adding bloatware at the code.  
> 
> OK.
> 
> > Please remember that
> > we want one logical change per patch.
> >
> > It means that, if you add a state->frontend_status at the driver, the
> > patch should implement the entire logic for it.  
> I will keep this in mind.
> 
> >   static int dvb_dummy_fe_sleep(struct dvb_frontend* fe)
> >   {
> > +
> > +	struct dvb_dummy_fe_state *state = fe->demodulator_priv;
> > +
> > +	state->sleeping = true;
> > +  
> 
> > Hmm...what's the sense of adding it? Where are you setting it to false?
> > Where are you using the sleeping state?  
> 
> I noted some drivers seem to instruct the hardware into a low power state. Take helene, for instance:
> 
> static int helene_sleep(struct dvb_frontend *fe)
> {
> 	struct helene_priv *priv = fe->tuner_priv;
> 
> 	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
> 	helene_enter_power_save(priv);
> 	return 0;
> }
> 
> static int helene_enter_power_save(struct helene_priv *priv)
> {
> 	dev_dbg(&priv->i2c->dev, "%s()\n", __func__);
> 	if (priv->state == STATE_SLEEP)
> 		return 0;
> 
> 	/* Standby setting for CPU */
> 	helene_write_reg(priv, 0x88, 0x0);
> 
> 	/* Standby setting for internal logic block */
> 	helene_write_reg(priv, 0x87, 0xC0);
> 
> 	priv->state = STATE_SLEEP;
> 	return 0;
> }
> 
> As we are emulating some piece of hardware I thought it would be enough to add a simple bool to indicate that the device was sleeping.

Yeah, that could have some value. Yet, if you're doing that, I suggest
to add a function to change the device power state, as this is what a
real driver would do. Something similar to:

static inline void change_power_state(struct state *st, bool on) 
{
	/* A real driver would have commands here to wake/sleep the dev */

	st->power = on;
}

In any case, a real device on sleeping mode will not be tuning any
channels, so all stats will reflect that, e. g:

	- frontend status will return 0;
	- stats that depends on having a lock will be set with:
		FE_SCALE_NOT_AVAILABLE
	  tip: most stats are like that
	- stats like signal strength should probably return 0.

Not so sure what SNR will return, but probably it should return
FE_SCALE_NOT_AVAILABLE too, as this is usually calculated indirectly
once the device is locked.

On other words, only signal strength and stats should return a value
with would be 0 on both cases. All other stats should return
FE_SCALE_NOT_AVAILABLE.

Btw, as this depends on having the stats implemented, I would suggest you to
first finish the tuning and stats part of the driver. Only after having those
patches, apply the power mode patch.

> 
> This patch was a bit convoluted on my part. Let's iron out the issues you pointed out in v2.
> 
> - Daniel.

Cheers,
Mauro