[2/8] DVB: dtv_property_cache_submit shouldn't modifiy the cache

  - Use const pointers and remove assignments.
- delivery_system already gets assigned by DTV_DELIVERY_SYSTEM
  and dtv_property_cache_sync.

Signed-off-by: Andreas Oberritter <obi@linuxtv.org>
---
 drivers/media/dvb/dvb-core/dvb_frontend.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)
  

Em 09-05-2011 01:03, Andreas Oberritter escreveu:
> - Use const pointers and remove assignments.

That's OK.

> - delivery_system already gets assigned by DTV_DELIVERY_SYSTEM
>   and dtv_property_cache_sync.

The logic for those legacy params is too complex. It is easy that
a latter patch to break the implicit set via dtv_property_cache_sync().

Do you actually see a bug caused by the extra set for the delivery system?
If not, I prefer to keep this extra re-assignment.
> 
> Signed-off-by: Andreas Oberritter <obi@linuxtv.org>
> ---
>  drivers/media/dvb/dvb-core/dvb_frontend.c |   13 +++----------
>  1 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c b/drivers/media/dvb/dvb-core/dvb_frontend.c
> index be0f631..1ac7633 100644
> --- a/drivers/media/dvb/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
> @@ -1074,7 +1074,7 @@ static void dtv_property_cache_sync(struct dvb_frontend *fe,
>   */
>  static void dtv_property_legacy_params_sync(struct dvb_frontend *fe)
>  {
> -	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> +	const struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>  	struct dvb_frontend_parameters *p = &fepriv->parameters;
>  
> @@ -1086,14 +1086,12 @@ static void dtv_property_legacy_params_sync(struct dvb_frontend *fe)
>  		dprintk("%s() Preparing QPSK req\n", __func__);
>  		p->u.qpsk.symbol_rate = c->symbol_rate;
>  		p->u.qpsk.fec_inner = c->fec_inner;
> -		c->delivery_system = SYS_DVBS;
>  		break;
>  	case FE_QAM:
>  		dprintk("%s() Preparing QAM req\n", __func__);
>  		p->u.qam.symbol_rate = c->symbol_rate;
>  		p->u.qam.fec_inner = c->fec_inner;
>  		p->u.qam.modulation = c->modulation;
> -		c->delivery_system = SYS_DVBC_ANNEX_AC;
>  		break;
>  	case FE_OFDM:
>  		dprintk("%s() Preparing OFDM req\n", __func__);
> @@ -1111,15 +1109,10 @@ static void dtv_property_legacy_params_sync(struct dvb_frontend *fe)
>  		p->u.ofdm.transmission_mode = c->transmission_mode;
>  		p->u.ofdm.guard_interval = c->guard_interval;
>  		p->u.ofdm.hierarchy_information = c->hierarchy;
> -		c->delivery_system = SYS_DVBT;
>  		break;
>  	case FE_ATSC:
>  		dprintk("%s() Preparing VSB req\n", __func__);
>  		p->u.vsb.modulation = c->modulation;
> -		if ((c->modulation == VSB_8) || (c->modulation == VSB_16))
> -			c->delivery_system = SYS_ATSC;
> -		else
> -			c->delivery_system = SYS_DVBC_ANNEX_B;
>  		break;
>  	}
>  }
> @@ -1129,7 +1122,7 @@ static void dtv_property_legacy_params_sync(struct dvb_frontend *fe)
>   */
>  static void dtv_property_adv_params_sync(struct dvb_frontend *fe)
>  {
> -	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> +	const struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>  	struct dvb_frontend_parameters *p = &fepriv->parameters;
>  
> @@ -1170,7 +1163,7 @@ static void dtv_property_adv_params_sync(struct dvb_frontend *fe)
>  
>  static void dtv_property_cache_submit(struct dvb_frontend *fe)
>  {
> -	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> +	const struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>  
>  	/* For legacy delivery systems we don't need the delivery_system to
>  	 * be specified, but we populate the older structures from the cache

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Em 09-05-2011 05:58, Mauro Carvalho Chehab escreveu:
> Em 09-05-2011 01:03, Andreas Oberritter escreveu:
>> - Use const pointers and remove assignments.
> 
> That's OK.
> 
>> - delivery_system already gets assigned by DTV_DELIVERY_SYSTEM
>>   and dtv_property_cache_sync.
> 
> The logic for those legacy params is too complex. It is easy that
> a latter patch to break the implicit set via dtv_property_cache_sync().
> 
> Do you actually see a bug caused by the extra set for the delivery system?
> If not, I prefer to keep this extra re-assignment.

Hmm... after applying all patches the logic change, and patch 2 may actually
make sense. I'll need to re-examine the patch series. 

On a quick look, if applied as-is, I suspect that git bisect
will break dvb in the middle of the patch series.

Anyway, patch 1/8 is OK. For now, I'll apply only this patch. I'll delay the
others until I have more time. I'm currently traveling abroad, due to Linaro
Development Summit, so, I don't have much time for review (and I'm also suffering
for a 5 hours jet-leg).

>>
>> Signed-off-by: Andreas Oberritter <obi@linuxtv.org>
>> ---
>>  drivers/media/dvb/dvb-core/dvb_frontend.c |   13 +++----------
>>  1 files changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c b/drivers/media/dvb/dvb-core/dvb_frontend.c
>> index be0f631..1ac7633 100644
>> --- a/drivers/media/dvb/dvb-core/dvb_frontend.c
>> +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
>> @@ -1074,7 +1074,7 @@ static void dtv_property_cache_sync(struct dvb_frontend *fe,
>>   */
>>  static void dtv_property_legacy_params_sync(struct dvb_frontend *fe)
>>  {
>> -	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>> +	const struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>>  	struct dvb_frontend_parameters *p = &fepriv->parameters;
>>  
>> @@ -1086,14 +1086,12 @@ static void dtv_property_legacy_params_sync(struct dvb_frontend *fe)
>>  		dprintk("%s() Preparing QPSK req\n", __func__);
>>  		p->u.qpsk.symbol_rate = c->symbol_rate;
>>  		p->u.qpsk.fec_inner = c->fec_inner;
>> -		c->delivery_system = SYS_DVBS;
>>  		break;
>>  	case FE_QAM:
>>  		dprintk("%s() Preparing QAM req\n", __func__);
>>  		p->u.qam.symbol_rate = c->symbol_rate;
>>  		p->u.qam.fec_inner = c->fec_inner;
>>  		p->u.qam.modulation = c->modulation;
>> -		c->delivery_system = SYS_DVBC_ANNEX_AC;
>>  		break;
>>  	case FE_OFDM:
>>  		dprintk("%s() Preparing OFDM req\n", __func__);
>> @@ -1111,15 +1109,10 @@ static void dtv_property_legacy_params_sync(struct dvb_frontend *fe)
>>  		p->u.ofdm.transmission_mode = c->transmission_mode;
>>  		p->u.ofdm.guard_interval = c->guard_interval;
>>  		p->u.ofdm.hierarchy_information = c->hierarchy;
>> -		c->delivery_system = SYS_DVBT;
>>  		break;
>>  	case FE_ATSC:
>>  		dprintk("%s() Preparing VSB req\n", __func__);
>>  		p->u.vsb.modulation = c->modulation;
>> -		if ((c->modulation == VSB_8) || (c->modulation == VSB_16))
>> -			c->delivery_system = SYS_ATSC;
>> -		else
>> -			c->delivery_system = SYS_DVBC_ANNEX_B;
>>  		break;
>>  	}
>>  }
>> @@ -1129,7 +1122,7 @@ static void dtv_property_legacy_params_sync(struct dvb_frontend *fe)
>>   */
>>  static void dtv_property_adv_params_sync(struct dvb_frontend *fe)
>>  {
>> -	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>> +	const struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>>  	struct dvb_frontend_parameters *p = &fepriv->parameters;
>>  
>> @@ -1170,7 +1163,7 @@ static void dtv_property_adv_params_sync(struct dvb_frontend *fe)
>>  
>>  static void dtv_property_cache_submit(struct dvb_frontend *fe)
>>  {
>> -	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>> +	const struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>>  
>>  	/* For legacy delivery systems we don't need the delivery_system to
>>  	 * be specified, but we populate the older structures from the cache
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 05/09/2011 06:12 AM, Mauro Carvalho Chehab wrote:
> Em 09-05-2011 05:58, Mauro Carvalho Chehab escreveu:
>> Em 09-05-2011 01:03, Andreas Oberritter escreveu:
>>> - Use const pointers and remove assignments.
>>
>> That's OK.
>>
>>> - delivery_system already gets assigned by DTV_DELIVERY_SYSTEM
>>>   and dtv_property_cache_sync.
>>
>> The logic for those legacy params is too complex. It is easy that
>> a latter patch to break the implicit set via dtv_property_cache_sync().
>>
>> Do you actually see a bug caused by the extra set for the delivery system?
>> If not, I prefer to keep this extra re-assignment.

No, but I was suprised to see the dtv_frontend_properties getting
modified in this function. There are three functions converting between
old and new structures:

dtv_property_cache_sync:
  converts from dvb_frontend_parameters to dtv_frontend_properties

dtv_property_legacy_params_sync and dtv_property_adv_params_sync:
  convert from dtv_frontend_properties to dvb_frontend_parameters

Assigning to fields of a source structure indicates a logical error. I
haven't found any comment on why this should be needed in the Git
history or in the mailing list archives.

Btw., I think that two functions should be enough. Any reason for not
merging dtv_property_adv_params_sync into
dtv_property_legacy_params_sync and calling the latter unconditionally?

> Hmm... after applying all patches the logic change, and patch 2 may actually
> make sense. I'll need to re-examine the patch series. 
> 
> On a quick look, if applied as-is, I suspect that git bisect
> will break dvb in the middle of the patch series.

Patches 6 and 8 depend on the patches mentioned in the cover letter. All
other patches should apply and compile cleanly. Which patch do you
suspect to break bisectability?

> Anyway, patch 1/8 is OK. For now, I'll apply only this patch. I'll delay the
> others until I have more time. I'm currently traveling abroad, due to Linaro
> Development Summit, so, I don't have much time for review (and I'm also suffering
> for a 5 hours jet-leg).

OK, there's no hurry.

Regards,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html