Re: [PATCH 1/7] block: add a cdrom_device_info pointer to struct gendisk

From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>
Cc: Tim Waugh <tim@cyberelk.net>, Borislav Petkov <bp@alien8.de>,
	Jan Kara <jack@suse.com>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/7] block: add a cdrom_device_info pointer to struct gendisk
Date: Thu, 23 Apr 2020 07:40:40 +0000	[thread overview]
Message-ID: <BY5PR04MB6900D254672137CF0FA07136E7D30@BY5PR04MB6900.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20200423071224.500849-2-hch@lst.de

On 2020/04/23 16:15, Christoph Hellwig wrote:
> Add a pointer to the CDROM information structure to struct gendisk.
> This will allow various removable media file systems to call directly
> into the CDROM layer instead of abusing ioctls with kernel pointers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/block/paride/pcd.c | 2 +-
>  drivers/cdrom/cdrom.c      | 5 ++++-
>  drivers/cdrom/gdrom.c      | 2 +-
>  drivers/ide/ide-cd.c       | 3 +--
>  drivers/scsi/sr.c          | 3 +--
>  include/linux/cdrom.h      | 2 +-
>  include/linux/genhd.h      | 9 +++++++++
>  7 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
> index cda5cf917e9a..5124eca90e83 100644
> --- a/drivers/block/paride/pcd.c
> +++ b/drivers/block/paride/pcd.c
> @@ -1032,7 +1032,7 @@ static int __init pcd_init(void)
>  
>  	for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++) {
>  		if (cd->present) {
> -			register_cdrom(&cd->info);
> +			register_cdrom(cd->disk, &cd->info);
>  			cd->disk->private_data = cd;
>  			add_disk(cd->disk);
>  		}
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index faca0f346fff..a1d2112fd283 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -586,7 +586,7 @@ static int cdrom_mrw_set_lba_space(struct cdrom_device_info *cdi, int space)
>  	return 0;
>  }
>  
> -int register_cdrom(struct cdrom_device_info *cdi)
> +int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi)
>  {
>  	static char banner_printed;
>  	const struct cdrom_device_ops *cdo = cdi->ops;
> @@ -601,6 +601,9 @@ int register_cdrom(struct cdrom_device_info *cdi)
>  		cdrom_sysctl_register();
>  	}
>  
> +	cdi->disk = disk;
> +	disk->cdi = cdi;
> +
>  	ENSURE(cdo, drive_status, CDC_DRIVE_STATUS);
>  	if (cdo->check_events == NULL && cdo->media_changed == NULL)
>  		WARN_ON_ONCE(cdo->capability & (CDC_MEDIA_CHANGED | CDC_SELECT_DISC));
> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
> index c51292c2a131..09b0cd292720 100644
> --- a/drivers/cdrom/gdrom.c
> +++ b/drivers/cdrom/gdrom.c
> @@ -770,7 +770,7 @@ static int probe_gdrom(struct platform_device *devptr)
>  		goto probe_fail_no_disk;
>  	}
>  	probe_gdrom_setupdisk();
> -	if (register_cdrom(gd.cd_info)) {
> +	if (register_cdrom(gd.disk, gd.cd_info)) {
>  		err = -ENODEV;
>  		goto probe_fail_cdrom_register;
>  	}
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index dcf8b51b47fd..40e124eb918a 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1305,8 +1305,7 @@ static int ide_cdrom_register(ide_drive_t *drive, int nslots)
>  	if (drive->atapi_flags & IDE_AFLAG_NO_SPEED_SELECT)
>  		devinfo->mask |= CDC_SELECT_SPEED;
>  
> -	devinfo->disk = info->disk;
> -	return register_cdrom(devinfo);
> +	return register_cdrom(info->disk, devinfo);
>  }
>  
>  static int ide_cdrom_probe_capabilities(ide_drive_t *drive)
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index d2fe3fa470f9..f9b589d60a46 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -794,9 +794,8 @@ static int sr_probe(struct device *dev)
>  	set_capacity(disk, cd->capacity);
>  	disk->private_data = &cd->driver;
>  	disk->queue = sdev->request_queue;
> -	cd->cdi.disk = disk;
>  
> -	if (register_cdrom(&cd->cdi))
> +	if (register_cdrom(disk, &cd->cdi))
>  		goto fail_put;
>  
>  	/*
> diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
> index 528271c60018..4f74ce050253 100644
> --- a/include/linux/cdrom.h
> +++ b/include/linux/cdrom.h
> @@ -104,7 +104,7 @@ extern unsigned int cdrom_check_events(struct cdrom_device_info *cdi,
>  				       unsigned int clearing);
>  extern int cdrom_media_changed(struct cdrom_device_info *);
>  
> -extern int register_cdrom(struct cdrom_device_info *cdi);
> +extern int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi);
>  extern void unregister_cdrom(struct cdrom_device_info *cdi);
>  
>  typedef struct {
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 058d895544c7..f9c226f9546a 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -217,11 +217,20 @@ struct gendisk {
>  #ifdef  CONFIG_BLK_DEV_INTEGRITY
>  	struct kobject integrity_kobj;
>  #endif	/* CONFIG_BLK_DEV_INTEGRITY */
> +#if IS_ENABLED(CONFIG_CDROM)
> +	struct cdrom_device_info *cdi;
> +#endif
>  	int node_id;
>  	struct badblocks *bb;
>  	struct lockdep_map lockdep_map;
>  };
>  
> +#if IS_REACHABLE(CONFIG_CDROM)
> +#define disk_to_cdi(disk)	((disk)->cdi)
> +#else
> +#define disk_to_cdi(disk)	NULL
> +#endif
> +
>  static inline struct gendisk *part_to_disk(struct hd_struct *part)
>  {
>  	if (likely(part)) {
> 

Looks OK to me.

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

-- 
Damien Le Moal
Western Digital Research