Re: [NDCTL PATCH v3] cxl/region: Add -f option for disable-region

["Cao, Quanquan/曹 全全" <caoqq@fujitsu.com>]

> +static int disable_region(struct cxl_region *region)
> +{
> +	const char *devname = cxl_region_get_devname(region);
> +	struct daxctl_region *dax_region;
> +	struct daxctl_memory *mem;
> +	struct daxctl_dev *dev;
> +	int failed = 0, rc;
> +
> +	dax_region = cxl_region_get_daxctl_region(region);
> +	if (!dax_region)
> +		goto out;
> +
> +	daxctl_dev_foreach(dax_region, dev) {
> +		mem = daxctl_dev_get_memory(dev);
> +		if (!mem)
> +			return -ENXIO;
> +
> +		/*
> +		 * If memory is still online and user wants to force it, attempt
> +		 * to offline it.
> +		 */
> +		if (daxctl_memory_is_online(mem)) {
> +			rc = daxctl_memory_offline(mem);
> +			if (rc < 0) {
> +				log_err(&rl, "%s: unable to offline %s: %s\n",
> +					devname,
> +					daxctl_dev_get_devname(dev),
> +					strerror(abs(rc)));
> +				if (!param.force)
> +					return rc;
> +
> +				failed++;
> +			}
> +		}
> +	}
> +
> +	if (failed) {
> +		log_err(&rl, "%s: Forcing region disable without successful offline.\n",
> +			devname);
> +		log_err(&rl, "%s: Physical address space has now been permanently leaked.\n",
> +			devname);
> +		log_err(&rl, "%s: Leaked address cannot be recovered until a reboot.\n",
> +			devname);
> +	}
> +

>   static int do_region_xable(struct cxl_region *region, enum region_actions action)
>   {
>   	switch (action) {
>   	case ACTION_ENABLE:
>   		return cxl_region_enable(region);
>   	case ACTION_DISABLE:
> -		return cxl_region_disable(region);
> +		return disable_region(region);
>   	case ACTION_DESTROY:
>   		return destroy_region(region);
>   	default:

Hi Dave

In this patch, a new function 'disable_region(region)' has been added. 
When using the 'cxl destroy-region region0 -f' command, there's a check 
first, followed by the 'destroy-region' operation. In terms of 
user-friendliness, which function is more user-friendly: 
'cxl_region_disable(region)' or 'disable_region(region)'?

Attach destroy_region section code
static int destroy_region(struct cxl_region *region)
{
     const char *devname = cxl_region_get_devname(region);
     unsigned int ways, i;
     int rc;

     /* First, unbind/disable the region if needed */
     if (cxl_region_is_enabled(region)) {
         if (param.force) {
             rc = cxl_region_disable(region);
             if (rc) {
                 log_err(&rl, "%s: error disabling region: %s\n",
                     devname, strerror(-rc));
                 return rc;
             }
         } else {
             log_err(&rl, "%s active. Disable it or use --force\n",
                 devname);
             return -EBUSY;
         }
     }

I have considered two options for your reference:

1.Assuming the user hasn't executed the 'cxl disable-region region0' 
command and directly runs 'cxl destroy-region region0 -f', using the 
'disable_region(region)' function to first take the region offline and 
then disable it might be more user-friendly.
2.If the user executes the 'cxl disable-region region0' command but 
fails to take it offline successfully, then runs 'cxl destroy-region 
region0 -f', using the 'cxl_region_disable(region)' function to directly 
'disable region' and then 'destroy region' would also be reasonable.