[linux-dvb] [patch 2.6.11-rc3 2/3] av7110: janitoring in attach/detach

[Johannes Stezenbach]

On Thu, Feb 10, 2005 at 12:57:12AM +0100, Francois Romieu wrote:
> Janitoring - error handling during attach
> - av7110_arm_sync(): small helper to factor out some code;
> - av7110_attach() does not check the status code returned by all the
>   functions is uses;
> - balance the error path in av7110_attach and have it easy to check.
>   Please check it;
> - if everything is correctly balanced, device_initialized is not needed
>   anymore in struct av7110;
> - av7110_detach(): no need to cast a void * pointer;
> - av7110_detach(): die #ifdef, die !
> - change the returned value of av7110_av_exit() as it can't fail;
> - change the returned value of av7110_ca_init() as it can fail. Removed
>   extraneous casts while are it.

Very good, but the patch doesn't apply (4 out of 14 hunks FAILED
for av7110.c). Could you please send an updated patch?

> -	ttpci_eeprom_parse_mac(&av7110->i2c_adap, av7110->dvb_adapter->proposed_mac);
> +	ret = ttpci_eeprom_parse_mac(&av7110->i2c_adap,
> +				     av7110->dvb_adapter->proposed_mac);
> +	if (ret < 0)
> +		goto err_i2c_del_3;

Not good. I don't think a wrong eeprom checksum should keep
people from using the card.

> +	for (i = 0; i < 2; i++) {
> +		struct ipack *ipack = av7110->ipack + i;
> +
> +		ret = av7110_ipack_init(ipack, IPACKS, play[i]);
> +		if (ret < 0) {
> +			while (i--)
> +				av7110_ipack_free(ipack);
> +			goto out;
> +		}
> +		ipack->data = av7110;
> +	}

av7110_ipack_free() would free the wrong ipack. Also, IMHO the loop
is superflous since we'll never have more than two ipacks.

>  static int ci_ll_init(struct dvb_ringbuffer *cirbuf, struct dvb_ringbuffer *ciwbuf, int size)
>  {
> +	/* TODO: is it legal to not check the value returned by vmalloc ? */
>  	dvb_ringbuffer_init(cirbuf, vmalloc(size), size);
>  	dvb_ringbuffer_init(ciwbuf, vmalloc(size), size);

For sure the vmalloc() return value must be checked.

Johannes