On 11/27/23 02:34, Cao, Quanquan/曹 全全 wrote:
>
>
>> +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.
To make the behavior consistent, I think we should use disable_region() with
the check for the destroy_region() path.
What do you think Vishal?
>
>
>
>
>
