Re: [PATCH 5/7] hfsplus: stop using ioctl_by_bdev

[Damien Le Moal]

On 2020/04/23 16:16, Christoph Hellwig wrote:
> Instead just call the CD-ROM layer functionality directly.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>  fs/hfsplus/wrapper.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
> index 08c1580bdf7a..61eec628805d 100644
> --- a/fs/hfsplus/wrapper.c
> +++ b/fs/hfsplus/wrapper.c
> @@ -127,31 +127,34 @@ static int hfsplus_read_mdb(void *bufptr, struct hfsplus_wd *wd)
>  static int hfsplus_get_last_session(struct super_block *sb,
>  				    sector_t *start, sector_t *size)
>  {
> -	struct cdrom_multisession ms_info;
> -	struct cdrom_tocentry te;
> -	int res;
> +	struct cdrom_device_info *cdi = disk_to_cdi(sb->s_bdev->bd_disk);
>  
>  	/* default values */
>  	*start = 0;
>  	*size = i_size_read(sb->s_bdev->bd_inode) >> 9;
>  
>  	if (HFSPLUS_SB(sb)->session >= 0) {
> +		struct cdrom_tocentry te;
> +
> +		if (!cdi)
> +			return -EINVAL;
> +
>  		te.cdte_track = HFSPLUS_SB(sb)->session;
>  		te.cdte_format = CDROM_LBA;
> -		res = ioctl_by_bdev(sb->s_bdev,
> -			CDROMREADTOCENTRY, (unsigned long)&te);
> -		if (!res && (te.cdte_ctrl & CDROM_DATA_TRACK) == 4) {
> -			*start = (sector_t)te.cdte_addr.lba << 2;
> -			return 0;
> +		if (cdrom_read_tocentry(cdi, &te) ||
> +		    (te.cdte_ctrl & CDROM_DATA_TRACK) != 4) {
> +			pr_err("invalid session number or type of track\n");
> +			return -EINVAL;
>  		}
> -		pr_err("invalid session number or type of track\n");
> -		return -EINVAL;
> +		*start = (sector_t)te.cdte_addr.lba << 2;
> +	} else if (cdi) {
> +		struct cdrom_multisession ms_info;
> +
> +		ms_info.addr_format = CDROM_LBA;
> +		if (cdrom_multisession(cdi, &ms_info) == 0 && ms_info.xa_flag)
> +			*start = (sector_t)ms_info.addr.lba << 2;
>  	}
> -	ms_info.addr_format = CDROM_LBA;
> -	res = ioctl_by_bdev(sb->s_bdev, CDROMMULTISESSION,
> -		(unsigned long)&ms_info);
> -	if (!res && ms_info.xa_flag)
> -		*start = (sector_t)ms_info.addr.lba << 2;
> +

White space change here, but I think it's ok. I do like a blank line before
return :)

>  	return 0;
>  }
>  
> 

Looks OK to me.

Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxx>


-- 
Damien Le Moal
Western Digital Research